Closed Bug 1470910 Opened 6 years ago Closed 6 years ago

Migrate input-box / input-box-spell to a Custom Element

Categories

(Toolkit :: UI Widgets, task, P3)

Product:
Toolkit
Component:
UI Widgets
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1470910 - Migrate input-box and input-box-spell to a Custom Element;
59 bytes, text/x-review-board-request
bgrins
: review+
Details 
These are attached to .textbox-input-box which are hbox elements used inside of textbox bindings (plus overrides for search bar and url bar). I tested this out a bit locally and we can convert input-box to a Custom Element using a new tag name. Depending on what use-cases we need to support for the spellcheck mode it could be folded into the main Custom Element or defined as another one.
It looks like there are a couple of places that use the spellcheck attribute on <textbox>: https://searchfox.org/comm-central/search?q=spellcheck%22%2C+&path=. Can you confirm they still work with this patch? That feature is only lightly tested in m-c.

There shouldn't need to be any changes to <textbox> consumers. Although in order to support the Custom Element, any place where `<xul:hbox class="textbox-input-box>` is explicitly created will need to be updated to `<xul:textbox-input-box>` (or to limit CSS changes `<xul:textbox-input-box class="textbox-input-box">`): https://searchfox.org/comm-central/search?q=class%3D%22textbox-input-box&path==.
Flags: needinfo?(jorgk)
Priority: -- → P3
This is green except for an accessibility test on of all things a <menulist editable=true>: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d34df160bc0b4e9bb5237b4222624cfe6890031a&group_state=expanded&selectedJob=184727084. Going to look into that but it should still be ready for review.
Comment on attachment 8987565 [details]
Bug 1470910 - Migrate input-box and input-box-spell to a Custom Element;

https://reviewboard.mozilla.org/r/252806/#review259456

::: browser/base/content/urlbarBindings.xml:94
(Diff revision 2)
>          this.inputField.addEventListener("overflow", this);
>          this.inputField.addEventListener("underflow", this);
>  
>          var textBox = document.getAnonymousElementByAttribute(this,
>                                                  "anonid", "textbox-input-box");
> -        var cxmenu = document.getAnonymousElementByAttribute(textBox,
> +        customElements.upgrade(textBox);

This line can be removed if Bug 1470242 lands ahead of this.
(In reply to Brian Grinstead [:bgrins] from comment #3)
> It looks like there are a couple of places that use the spellcheck attribute
> on <textbox>:
> https://searchfox.org/comm-central/search?q=spellcheck%22%2C+&path=. Can you
> confirm they still work with this patch? That feature is only lightly tested
> in m-c.
Sorry about the late reply, I was busy with beta and merge day issues.

Frankly, I don't understand the question. The query you provided gives two hits on C-C:
chat/modules/imTextboxUtils.jsm
139	aTextbox.setAttribute("spellcheck", "true");
suite/mailnews/compose/MsgComposeCommands.js
3450	GetMsgSubjectElement().setAttribute("spellcheck", aEnableInlineSpellCheck);
and a whole heap on M-C.

So what it the problem with that? Or is it about the second part of your question:
https://searchfox.org/comm-central/search?q=class%3D%22textbox-input-box&path
Yes, that shows a few hits in M-C-removed bindings we forked to C-C/common.

I'll pass this question on to Richard. Richard, please see comment #3. Also, please file a bug for possible necessary C-C changes.
Flags: needinfo?(jorgk) → needinfo?(richard.marti)
All in all this works for me, but:
With your patches applied, and the change on our side to the xul:textbox-input-box, I see on a normal textbox with spellcheck (the subject field in compose window), when I mistype a word, the red underline but in the context menu under "Languages" no popup with the installed languages. Is this a bug in one of your patches or do we need to change more on our side?
Flags: needinfo?(richard.marti)
(In reply to Richard Marti (:Paenglab) from comment #9)
> All in all this works for me, but:
> With your patches applied, and the change on our side to the
> xul:textbox-input-box, I see on a normal textbox with spellcheck (the
> subject field in compose window), when I mistype a word, the red underline
> but in the context menu under "Languages" no popup with the installed
> languages. Is this a bug in one of your patches or do we need to change more
> on our side?

It was a bug in the patch (was passing the wrong node to addDictionaryListToMenu). Can you confirm with the latest version that it works?
Flags: needinfo?(richard.marti)
Yes, the menu is working again. Thanks.
Flags: needinfo?(richard.marti)
(In reply to Brian Grinstead [:bgrins] from comment #6)
> This is green except for an accessibility test on of all things a <menulist
> editable=true>:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d34df160bc0b4e9bb5237b4222624cfe6890031a&group_state=e
> xpanded&selectedJob=184727084. Going to look into that but it should still
> be ready for review.

Figured it out. I had just forgotten to update an <hbox> to a <textbox-input-box> in the menulist-editable binding: https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/toolkit/content/widgets/menulist.xml#359.
Comment on attachment 8987565 [details]
Bug 1470910 - Migrate input-box and input-box-spell to a Custom Element;

https://reviewboard.mozilla.org/r/252806/#review260008

This may take a while, since it's complex even if some code has only been moved. It requires care since the case without "spellcheck" is used extensively, the upside is that probably we have good test coverage.

Do you have a Talos run to see if performance is different from the XBL version?

I'd hoped we could separate the "spellcheck" case into a subclass, but it may not be easy given how this is constructed directly in other XBL bindings, also with child elements. This is something we may have to think about for other conversions. Do you know if inheriting the "is" attribute works at all, like <sub-component inherits="is=subcomponentis">? For sure, it cannot be set later in the JavaScript code.

::: toolkit/content/customElements.js:40
(Diff revision 4)
>     *        String with the XML representation of XUL elements.
>     *
>     * @return DocumentFragment containing the corresponding element tree, including
>     *         element nodes but excluding any text node.
>     */
> -  static parseXULToFragment(str) {
> +  static parseXULToFragment(str, entities = "") {

The new argument should be either a generic "preamble" parameter or an array of URLs that we can use to generate the list of entities. I'm leaning towards the latter, maybe as part of an "options" object argument, since it looks like several callers would need this and we can reduce code duplication a bit. In either case, it should be documented.
Comment on attachment 8987565 [details]
Bug 1470910 - Migrate input-box and input-box-spell to a Custom Element;

https://reviewboard.mozilla.org/r/252806/#review260844

::: toolkit/content/tests/chrome/test_textbox_dictionary.xul:78
(Diff revision 4)
>          var undoAddDict = contextMenu.querySelector("[anonid=spell-undo-add-to-dictionary]");
>          ok(!undoAddDict.hidden, "Is Undo Add to Dictioanry visible?");
>  
>          var separator = contextMenu.querySelector("[anonid=spell-suggestions-separator]");
>          ok(!separator.hidden, "Is separator hidden?");

This other case should also be converted to use getMenuItem.

There is also a reference to the context menu element in Marionette infrastructure:

https://dxr.mozilla.org/mozilla-central/rev/aea3f3457f1531706923b8d4c595a1f271de83da/testing/marionette/puppeteer/firefox/firefox_puppeteer/ui/browser/toolbars.py#127

For heavily used widgets, I suggest running a full set of tests like "-u all" in tryserver builds at least once, there may be other tests that we don't even think of that may rely on the widget.
(In reply to Brian Grinstead [:bgrins] from comment #18)
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=ca9d2b061d7fbbcdf5211f348319fef3
> 4105151e&newProject=try&newRevision=d31596b9f0a9fdae808433ff3dad2d856a6fa48e&
> framework=1&showOnlyImportant=1

I see a possible regression in "about_preferences_basic opt e10s stylo". I don't see a description of this test at <https://wiki.mozilla.org/Buildbot/Talos/Tests>, and I don't know if it's considered tier 1, but from the name I guess this is measuring the "about:preferences" loading performance?

If confirmed, maybe it means we don't have sufficient caching for in-content pages like we have for the main interface?
Depends on: 1470242
Comment on attachment 8987565 [details]
Bug 1470910 - Migrate input-box and input-box-spell to a Custom Element;

https://reviewboard.mozilla.org/r/252806/#review260892

There are enough comments for this round, and we need to figure out the situation in bug 1470242, so clearing review for now.

::: browser/base/content/urlbarBindings.xml:29
(Diff revision 4)
> -        <xul:hbox anonid="textbox-input-box"
> -                  class="textbox-input-box urlbar-input-box"
> +        <xul:textbox-input-box anonid="textbox-input-box"
> +                  class="urlbar-input-box"

For dashed Custom Elements, should we start using the "moz-" prefix?

If I remember correctly, the eventual plan for the migration to web technologies was to prefix with "moz-" all the elements that were formerly XUL, except we wouldn't rename some non-dashed built-in XUL elements, so we wouldn't have to change all the styling rules and the code that queries elements by tag name.

Since we don't have this issue here, maybe this should be named "moz-textbox-input-box", "moz-text-input", or "moz-input-box"?

::: toolkit/content/widgets/textbox.js:86
(Diff revision 4)
> +        </menupopup>
> +      `, entities);
> -          }
> +    }
>  
> -          return this.InlineSpellCheckerUI;
> -        ]]></getter>
> +    return MozXULElement.parseXULToFragment(`
> +      <menupopup anonid="input-box-contextmenu" class="textbox-contextmenu">

If possible, we should remove the anonid="input-box-contextmenu" and update the Marionette test if it relies on it.

In general, I guess that for Custom Elements using MozXULElement.parseXULToFragment we would still use a unique attribute to query a specific item we created, when we don't have a better attribute?

I'm wondering if the other "anonid" attributes should be renamed to something else, like "data-id", also to detect cases where we missed the conversion to the "getMenuItem" getter. On the other hand, reusing the "anonid" name appears to provide a degree of backwards compatibility, even though technically the element isn't anonymous anymore. What do you think?
Attachment #8987565 - Flags: review?(paolo.mozmail)
Comment on attachment 8987565 [details]
Bug 1470910 - Migrate input-box and input-box-spell to a Custom Element;

https://reviewboard.mozilla.org/r/252806/#review260898

::: toolkit/content/widgets/textbox.js:34
(Diff revision 4)
> -            hidden = !visible;
> -        ]]></body>
> -      </method>
> -
> -      <method name="doCommand">
> -        <parameter name="command"/>
> +    }
> +
> +    this.setAttribute("context", "_child");
> +    this.appendChild(this._getContextMenu());
> +
> +    this.menuPopup = this.querySelector(".textbox-contextmenu");

Also, the "menupopup" property is lowercase in other widgets, so it may be better to keep it lowercase for consistency.
Comment on attachment 8987565 [details]
Bug 1470910 - Migrate input-box and input-box-spell to a Custom Element;

https://reviewboard.mozilla.org/r/252806/#review260902

::: toolkit/content/widgets/textbox.js:78
(Diff revision 4)
> +        <menuseparator></menuseparator>
> +        <menuitem label="&selectAllCmd.label;" accesskey="&selectAllCmd.accesskey;" cmd="cmd_selectAll"></menuitem>
> +    `;
> +
> +    if (!this.spellcheck) {
> +      return MozXULElement.parseXULToFragment(`

I'm also wondering if we should use a lazy getter for the fragment, and then use document.importNode like I did in HandlerListItem. This keeps one extra copy of the fragment in memory for the lifetime of the document, but given how this element in particular has the potential of being created in repeated lists, it may be worth the performance benefit.
Depends on: 1473130
Comment on attachment 8987565 [details]
Bug 1470910 - Migrate input-box and input-box-spell to a Custom Element;

https://reviewboard.mozilla.org/r/252806/#review260844

> This other case should also be converted to use getMenuItem.
> 
> There is also a reference to the context menu element in Marionette infrastructure:
> 
> https://dxr.mozilla.org/mozilla-central/rev/aea3f3457f1531706923b8d4c595a1f271de83da/testing/marionette/puppeteer/firefox/firefox_puppeteer/ui/browser/toolbars.py#127
> 
> For heavily used widgets, I suggest running a full set of tests like "-u all" in tryserver builds at least once, there may be other tests that we don't even think of that may rely on the widget.

Updated the other references in this test, good catch
Comment on attachment 8987565 [details]
Bug 1470910 - Migrate input-box and input-box-spell to a Custom Element;

https://reviewboard.mozilla.org/r/252806/#review260892

> For dashed Custom Elements, should we start using the "moz-" prefix?
> 
> If I remember correctly, the eventual plan for the migration to web technologies was to prefix with "moz-" all the elements that were formerly XUL, except we wouldn't rename some non-dashed built-in XUL elements, so we wouldn't have to change all the styling rules and the code that queries elements by tag name.
> 
> Since we don't have this issue here, maybe this should be named "moz-textbox-input-box", "moz-text-input", or "moz-input-box"?

Done - renamed to "moz-input-box"

> If possible, we should remove the anonid="input-box-contextmenu" and update the Marionette test if it relies on it.
> 
> In general, I guess that for Custom Elements using MozXULElement.parseXULToFragment we would still use a unique attribute to query a specific item we created, when we don't have a better attribute?
> 
> I'm wondering if the other "anonid" attributes should be renamed to something else, like "data-id", also to detect cases where we missed the conversion to the "getMenuItem" getter. On the other hand, reusing the "anonid" name appears to provide a degree of backwards compatibility, even though technically the element isn't anonymous anymore. What do you think?

I'd also like to also get rid of the anonid name eventually, but not necessarily during initial conversion. First step is probably to update all references to point to an instance property on the CE, and then eventually change how we querySelector for it from within the element.

Marionette tests pose an additional challenge because we can't run the custom element getters so we'll need to continue to have something unique to query on.
Comment on attachment 8987565 [details]
Bug 1470910 - Migrate input-box and input-box-spell to a Custom Element;

https://reviewboard.mozilla.org/r/252806/#review260898

> Also, the "menupopup" property is lowercase in other widgets, so it may be better to keep it lowercase for consistency.

done
Comment on attachment 8987565 [details]
Bug 1470910 - Migrate input-box and input-box-spell to a Custom Element;

https://reviewboard.mozilla.org/r/252806/#review260902

> I'm also wondering if we should use a lazy getter for the fragment, and then use document.importNode like I did in HandlerListItem. This keeps one extra copy of the fragment in memory for the lifetime of the document, but given how this element in particular has the potential of being created in repeated lists, it may be worth the performance benefit.

Yeah, that may make sense. Since we have the spellcheck case as well (which checks an instance property), doing this optimization would require a bit of extra work. I'd like to see if there's a talos hit before making the change.
Eitan, I've typically asked Marco to test builds like this out for accessibility regressions (similar to https://bugzilla.mozilla.org/show_bug.cgi?id=1411707#c119), but it appears he's unavailable for a couple of weeks. Would you be able to help (or get the request redirected properly)?

For context, this change migrates the DOM node that wraps XUL textboxes [0] from XBL to a Custom Element. So pretty much any textbox (the URL bar, search bar, find bar, input fields in preferences, etc) is potentially affected. However, the wrapper node doesn't set any particular attributes [1] so I don't expect major regressions there.

The main function it has is setting up the context menu for the 'edit' controls, so the things that I'd like to confirm are:
1) That Undo/Cut/Copy/Paste/Delete/Select All from the context menu are still properly accessible.
2) The URL bar extends the context menu to add 'Paste & Go' in the middle of the menu, so that should work properly.

A build is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea87b0d1e0253559b9d6964404863e08c1e3e37b. Thanks in advance - let me know if you have any questions or issues running it.

[0]: Based on the .textbox-input-box selector, for example: https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/toolkit/content/widgets/textbox.xml#403
[1]: https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/toolkit/content/widgets/textbox.xml#410-472
Flags: needinfo?(eitan)
Hey Brian.

I did some basic testing, specifically on what you asked for and it looks good. I'm not sure what change in Past & Go I should look for, but the menu is functional. Perhaps a keyboard shortcut would be cool on that, but that is for a different bug.

I'm not a full-time screen reader user so I may be missing something. Worth waiting for Marco to come back and check it out.
Flags: needinfo?(eitan)
I (as a full-time screen reader user) tested this as suggested in comment 31. Specifically, I tested the address bar, the find bar and the search box in Preferences. To me, both Nightly and this build feel equivalent, both editing in the field itself and in the context menu. All editing items in the context menu work as expected, including Paste and Go in the address bar. I think we're good to go from an a11y standpoint. Thanks for checking!
Great, thank you both for testing!
Comment on attachment 8987565 [details]
Bug 1470910 - Migrate input-box and input-box-spell to a Custom Element;

https://reviewboard.mozilla.org/r/252806/#review267690

Thanks, r+ with the two questions below answered. Next time, for similar cases it may make review easier if we consolidate the bindings in one changeset and then convert them to Custom Elements separately.

What is the state of comment 20 and performance testing?

::: commit-message-41ee7:3
(Diff revision 6)
> +Instead of `<xul:hbox class="textbox-input-box">`, consumers now should use
> +`<xul:textbox-input-box />`. This covers the normal case and also handles
> +[spellcheck=true] while sharing much of the code within the single class.

The commit message should be updated.

::: browser/components/search/content/search.xml:508
(Diff revision 6)
>            this.setAttribute("clickSelectsAll", true);
>  
>          var textBox = document.getAnonymousElementByAttribute(this,
> -                                              "anonid", "textbox-input-box");
> -        var cxmenu = document.getAnonymousElementByAttribute(textBox,
> -                                          "anonid", "input-box-contextmenu");
> +                                              "anonid", "moz-input-box");
> +
> +        customElements.upgrade(textBox);

What is the situation of bug 1470242? According to bug 1470242 comment 14, last time these "upgrade" calls seemed unnecessary...

::: toolkit/content/widgets/textbox.js:9
(Diff revision 6)
>  
> -  <binding id="input-box">
> -    <content context="_child">
> -      <children/>
> -      <xul:menupopup anonid="input-box-contextmenu"
> -                     class="textbox-contextmenu"
> +"use strict";
> +
> +{
> +
> +class InputBox extends MozXULElement {

nit: I'd prefer MozInputBox, even if it's only a local name
Attachment #8987565 - Flags: review?(paolo.mozmail) → review+
(In reply to :Paolo Amadini from comment #35)
> ::: browser/components/search/content/search.xml:508
> (Diff revision 6)
> >            this.setAttribute("clickSelectsAll", true);
> >  
> >          var textBox = document.getAnonymousElementByAttribute(this,
> > -                                              "anonid", "textbox-input-box");
> > -        var cxmenu = document.getAnonymousElementByAttribute(textBox,
> > -                                          "anonid", "input-box-contextmenu");
> > +                                              "anonid", "moz-input-box");
> > +
> > +        customElements.upgrade(textBox);
> 
> What is the situation of bug 1470242? According to bug 1470242 comment 14,
> last time these "upgrade" calls seemed unnecessary...

I did a test where I removed the upgrade calls and am getting errors like: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c721289099e5d2fafd03562baee9a95006a52cc7&selectedJob=191471937
(In reply to Brian Grinstead [:bgrins] from comment #36)
> I did a test where I removed the upgrade calls and am getting errors like:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=c721289099e5d2fafd03562baee9a95006a52cc7&selectedJob=1
> 91471937

Okay, we know this may depend on the exact order of events, so it's possible that it's different between testing manually and in automation.

Anyways, I'd add a comment saying that this call is a workaround, and may become unnecessary once bug 1470242 is fixed. In general, we designed the loading mechanism to avoid situations where we need "upgrade" calls, because otherwise we can't rely on the element interface being consistent throughout its lifetime.
(In reply to :Paolo Amadini from comment #37)
> Anyways, I'd add a comment saying that this call is a workaround, and may
> become unnecessary once bug 1470242 is fixed. In general, we designed the
> loading mechanism to avoid situations where we need "upgrade" calls, because
> otherwise we can't rely on the element interface being consistent throughout
> its lifetime.

Good idea, I'll add a link to the bug. And yes, I'd much prefer if we could always assume the element is upgraded once we have a JS reference to it.
Attachment #8987565 - Attachment is obsolete: true
Mozreview cleared the r? flag when folding. That's unfortunate, since I was planning to land it.
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2058357d242c
Migrate input-box and input-box-spell to a Custom Element;r=paolo
Attachment #8987564 - Flags: review?(paolo.mozmail) → review+
I'm going to be away for a few days, but Paolo volunteered to help if any issues come up with this landing.
Depends on: 1480716
https://hg.mozilla.org/mozilla-central/rev/2058357d242c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: