Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add focus support to BrowserRenderer #17472

Closed
SQL-MisterMagoo opened this issue Nov 28, 2019 · 18 comments
Closed

Add focus support to BrowserRenderer #17472

SQL-MisterMagoo opened this issue Nov 28, 2019 · 18 comments
Labels
area-blazor Includes: Blazor, Razor Components Components Big Rock This issue tracks a big effort which can span multiple issues enhancement This issue represents an ask for new feature or an enhancement to an existing one
Milestone

Comments

@SQL-MisterMagoo
Copy link
Contributor

SQL-MisterMagoo commented Nov 28, 2019

Is your feature request related to a problem? Please describe.

To set focus on an element from Blazor, currently requires and ElementReference (or an id) and a JSInterop call in OnAfterRenderAsync.
It would be nice to be able to do this declaratively by using the "autofocus" attribute.

This is a capability of HTML, but does not work for SPA applications where the elements are inserted into an existing DOM.

The addition of an ability to have autofocus on newly created elements would make the SPA developer experience much simpler and provide a better result for the end user of the application.

Describe the solution you'd like

The Blazor application renders an element with the autofocus attribute and that triggers the BrowserRenderer to call the focus() method on the newly create element.

<button @onclick=@(MyClickHandler) autofocus>Click Me</button>

This should only cover initial element creation to maintain consistency with normal html autofocus.

Additional context

The BrowserRenderer used in Blazor can be modified in a manner similar to this (proof of concept testing confirms this at a superficial level) to provide autofocus on element creation.

private insertElement(batch: RenderBatch, componentId: number, parent: LogicalElement, childIndex: number, frames: ArrayValues<RenderTreeFrame>, frame: RenderTreeFrame, frameIndex: number) {
    const frameReader = batch.frameReader;
    const tagName = frameReader.elementName(frame)!;
    const newDomElementRaw = tagName === 'svg' || isSvgElement(parent) ?
      document.createElementNS('http://www.w3.org/2000/svg', tagName) :
      document.createElement(tagName);
    const newElement = toLogicalElement(newDomElementRaw);
    insertLogicalChild(newDomElementRaw, parent, childIndex);

+    // Handle autofocus
+    let wantsFocus: boolean = false;
    // Apply attributes
    const descendantsEndIndexExcl = frameIndex + frameReader.subtreeLength(frame);
    for (let descendantIndex = frameIndex + 1; descendantIndex < descendantsEndIndexExcl; descendantIndex++) {
      const descendantFrame = batch.referenceFramesEntry(frames, descendantIndex);
      if (frameReader.frameType(descendantFrame) === FrameType.attribute) {
        this.applyAttribute(batch, componentId, newDomElementRaw, descendantFrame);
+        // Handle autofocus
+        let attrName = batch.frameReader.attributeName(descendantFrame);
+        wantsFocus = ( attrName === 'autofocus' );
      } else {
        // As soon as we see a non-attribute child, all the subsequent child frames are
        // not attributes, so bail out and insert the remnants recursively
        this.insertFrameRange(batch, componentId, newElement, 0, frames, descendantIndex, descendantsEndIndexExcl);
        break;
      }
    }

+    if (wantsFocus) { // Handle autofocus
+      newDomElementRaw.focus();
+    }
    
    // We handle setting 'value' on a <select> in two different ways:
    // [1] When inserting a corresponding <option>, in case you're dynamically adding options
    // [2] After we finish inserting the <select>, in case the descendant options are being
    //     added as an opaque markup block rather than individually
    // Right here we implement [2]
    if (newDomElementRaw instanceof HTMLSelectElement && selectValuePropname in newDomElementRaw) {
      const selectValue = newDomElementRaw[selectValuePropname];
      newDomElementRaw.value = selectValue;
      delete newDomElementRaw[selectValuePropname];
    }
  }

Link to gist with full source : https://gist.github.com/SQL-MisterMagoo/949f2aff8aa0006ab6843bcedd14dd62/revisions

EDIT: 30/11/2019 Section below should be considered removed from this request as it was flawed

~~### Additional context
At this point in the code, it would be simple to add another case statement to handle autofocus

https://github.com/aspnet/AspNetCore/blob/32a2cc594363672ccfe7644a649f77a8bfc9c4a8/src/Components/Web.JS/src/Rendering/BrowserRenderer.ts#L311

  private tryApplySpecialProperty(batch: RenderBatch, element: Element, attributeName: string, attributeFrame: RenderTreeFrame | null) {
    switch (attributeName) {
      case 'value':
        return this.tryApplyValueProperty(batch, element, attributeFrame);
      case 'checked':
        return this.tryApplyCheckedProperty(batch, element, attributeFrame);

       /* ** Suggested addition ** */
      case 'autofocus': {
          element.focus();
          return true;
        }

      default: {
        if (attributeName.startsWith(internalAttributeNamePrefix)) {
          this.applyInternalAttribute(batch, element, attributeName.substring(internalAttributeNamePrefix.length), attributeFrame);
          return true;
        }
        return false;
      }
    }
  }
```~~
@javiercn javiercn added the area-blazor Includes: Blazor, Razor Components label Nov 28, 2019
@javiercn
Copy link
Member

@SQL-MisterMagoo thanks for contacting us.

We likely don't want to do this at the language/compiler level as it will unnecessarily complicate them. I believe this can be better handled at the component level, for example with your own input components.

@SQL-MisterMagoo
Copy link
Contributor Author

That's a real shame as all we can do from a Component is use interop , which forces us to wait until OnAfterRenderAsync and adds unnecessary overhead.

@SQL-MisterMagoo
Copy link
Contributor Author

@javiercn Does BrowserRenderer count as language/compiler code?

Isn't it a Blazor client side library that specifically handles the rendering/interaction of Blazor with the browser DOM, of which focus is a very fundamental requirement that Blazor does not handle.

I would love to give this some time for discussion - and would be entirely happy if there was a good alternative, but JSInterop doesn't really tick the boxes for something as fundamental as focus.

@SQL-MisterMagoo
Copy link
Contributor Author

Update: I just realised this doesn't cover setting focus the first time an element is rendered...I'll dig into it and see what that would require.

@egil
Copy link
Contributor

egil commented Nov 28, 2019

A more general solution could be to add the option to specific JavaScript to run immediately after a render of an element to the DOM is complete (which doesn't require another roundtrip/call from OnAfterRender). E.g. add attribute @onrender="js to run" and @onfirstrender="js to run".
This is especially useful in Blazor Server where network latency can vary greatly, and you as a component author want to make sure animation's and similar starts right away.

@SQL-MisterMagoo
Copy link
Contributor Author

I will come back to this - I have been able to test and it's not quite right...

@Lupusa87
Copy link
Contributor

If blazor can do PreventDefault and StopPropagation, same way set focus shouldn't be too hard.
I wish to have focus directive in blazor.

@egil
Copy link
Contributor

egil commented Nov 29, 2019

@Lupusa87 you cannot compare the two. stopPropagation and preventDefault were added because it was hard/impossible to achieve the effect otherwise. It's not so with focus.

@javiercn
Copy link
Member

We'll consider this feature during the next release planning period and update the status of this issue accordingly.

@javiercn javiercn added this to the Backlog milestone Nov 29, 2019
@Lupusa87
Copy link
Contributor

@Lupusa87 you cannot compare the two. stopPropagation and preventDefault were added because it was hard/impossible to achieve the effect otherwise. It's not so with focus.

You are right but it is not reason not to do focus from blazor.
I mentioned them to say that making focus from blazor could be possible and not too hard.

@egil
Copy link
Contributor

egil commented Nov 29, 2019

@Lupusa87 I understand. However, I much prefer if the Blazor team would add a general api that allows us to call any JavaScript function related to the just rendered element without requiring another roundtrip to the server.

Currently I am thinking the following triggers makes sense:

OnFirstRender
OnRender
OnDispose

The important point of that this should point to DOM element where the attributes are added. This will enable a the focus scenario in this issue to be easily implemented and many others too, without adding specific single scoped functionality.

@SQL-MisterMagoo
Copy link
Contributor Author

@ all I have modified the original message (I left in the original as people have "upvoted")

The reason was that I finally got around to testing and realised my original suggestion was unworkable.

This alternative is meant as a jumping off point - for cleverer people than me to discuss, but minimal testing shows it to work very nicely.

@egil I think a separate issue for your suggestion would be a good thing and I would be happy to support it, but we are in danger of discussing two slightly different things in one issue, I think. If they are two separate issues, one can be closed without affecting the other.

@egil
Copy link
Contributor

egil commented Nov 30, 2019

@SQL-MisterMagoo thanks, I agree. I'll create a separate issue after the weekend.

@mrpmorris
Copy link

This is how I achieved autofocus https://blazor-university.com/javascript-interop/calling-javascript-from-dotnet/passing-html-element-references/

@mkArtakMSFT mkArtakMSFT added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Jan 10, 2020
@fireinthemountain
Copy link

Including autofocus would be a great addition.

@mkArtakMSFT mkArtakMSFT added the Components Big Rock This issue tracks a big effort which can span multiple issues label Apr 28, 2020
@mkArtakMSFT
Copy link
Member

This has been resolved by #22892

@mkArtakMSFT
Copy link
Member

Given that we now have the Focus() support for ElementReference, we shouldn't do this any more.

@mrpmorris
Copy link

Having Focus makes it a little bit less work, but it will still be too much work. For example, writing a page with a tab control on it would be a hassle.

When you can just add autofocus, then we are more compliant with html, and it is no work at all.

Personally, I wrote a component to do this, but that's not a good experience for a new user.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components Components Big Rock This issue tracks a big effort which can span multiple issues enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

No branches or pull requests

9 participants