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)
Toolkit
UI Widgets
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: bgrins, Assigned: bgrins)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•6 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Comment 6•6 years ago
|
||
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.
Assignee | ||
Comment 7•6 years ago
|
||
mozreview-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.
Comment 8•6 years ago
|
||
(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)
Comment 9•6 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•6 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•6 years ago
|
||
(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.
Assignee | ||
Comment 16•6 years ago
|
||
Comment 17•6 years ago
|
||
mozreview-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/#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.
Assignee | ||
Comment 18•6 years ago
|
||
(In reply to :Paolo Amadini from comment #17)
> Do you have a Talos run to see if performance is different from the XBL
> version?
Good idea - here's a push: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=ca9d2b061d7fbbcdf5211f348319fef34105151e&newProject=try&newRevision=d31596b9f0a9fdae808433ff3dad2d856a6fa48e&framework=1&showOnlyImportant=1
Comment 19•6 years ago
|
||
mozreview-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/#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.
Comment 20•6 years ago
|
||
(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?
Comment 21•6 years ago
|
||
mozreview-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/#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 22•6 years ago
|
||
mozreview-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/#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 23•6 years ago
|
||
mozreview-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/#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.
Assignee | ||
Comment 24•6 years ago
|
||
mozreview-review-reply |
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
Assignee | ||
Comment 25•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 26•6 years ago
|
||
mozreview-review-reply |
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
Assignee | ||
Comment 27•6 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 31•6 years ago
|
||
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)
Comment 32•6 years ago
|
||
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)
Comment 33•6 years ago
|
||
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!
Assignee | ||
Comment 34•6 years ago
|
||
Great, thank you both for testing!
Comment 35•6 years ago
|
||
mozreview-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/#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+
Assignee | ||
Comment 36•6 years ago
|
||
(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
Comment 37•6 years ago
|
||
(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.
Assignee | ||
Comment 38•6 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8987565 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Assignee | ||
Comment 43•6 years ago
|
||
Mozreview cleared the r? flag when folding. That's unfortunate, since I was planning to land it.
Assignee | ||
Comment 44•6 years ago
|
||
I got a good looking talos compare https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=e9c2261c9c185284b3a406858027f78a218dc1b0&newProject=try&newRevision=35f2e124cb4c59250be6ec8ab490a2aadfa30fb4&framework=1&showOnlyImportant=1 and try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=35f2e124cb4c59250be6ec8ab490a2aadfa30fb4
Comment 45•6 years ago
|
||
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
Assignee | ||
Updated•6 years ago
|
Attachment #8987564 -
Flags: review?(paolo.mozmail) → review+
Assignee | ||
Comment 46•6 years ago
|
||
I'm going to be away for a few days, but Paolo volunteered to help if any issues come up with this landing.
Comment 47•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Updated•5 years ago
|
Type: enhancement → task
You need to log in
before you can comment on or make changes to this bug.
Description
•