1340643 - xul:button provide the ability to get access to the textNode that displays the resulting text

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect, P1)

Description

[Mike Conley (:mconley) (:⚙️)]

Example: any button. The button seems to manually paint the glyphs that make up the button label, and there's no textNode that we can use to add a DOM Range on for highlighting in search.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

For xul:label, we can do the following:
var textNode = label.firstChild;

For xul:checkbox, we can do the following:
var textNode = checkbox.boxObject.firstChild.nextSibling.firstChild.nextSibling.firstChild

However, for xul:button and xul:menulist, there does not appear to be a way to get to the inner textNode.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

xul:label's that use the value attribute instead of the textContent approach also do not have the textNode child.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Hey Neil, do you think you could help us out here? This is needed for adding a search function to about:preferences, and we would use it for creating selection ranges within the preferences to highlight search matches. The elements listed above don't have a way to get access to the textNodes within them, and thus we can't highlight the text.

Comment 4

[Neil Deakin]

xul:label doesn't support selection so you won't be able to do that. You would either need to use child text nodes in the labels, or you could add some kind of highlighting support to nsTextBoxFrame::DrawText.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

It seems the easiest route here is to update each of the <xul:label>s to use textContent instead of the value attribute.

Anytime we have a <label value="&someText.label;"/>, this should get converted to <label>&someText.label;</label>.

-----------------

As for xul:button, we should adjust the binding to have an extra xul:label element that sets the textContent.

What is currently:
>   <binding id="button" display="xul:button"
>            extends="chrome://global/content/bindings/button.xml#button-base">
>     <resources>
>       <stylesheet src="chrome://global/skin/button.css"/>
>     </resources>
> 
>     <content>
>       <children includes="observes|template|menupopup|panel|tooltip"/>
>       <xul:hbox class="box-inherit button-box" xbl:inherits="align,dir,pack,orient"
>                 align="center" pack="center" flex="1" anonid="button-box">
>         <children>
>           <xul:image class="button-icon" xbl:inherits="src=image"/>
>           <xul:label class="button-text" xbl:inherits="value=label,accesskey,crop"/>
>         </children>
>       </xul:hbox>
>     </content>
>   </binding>

Should become:
>   <binding id="button" display="xul:button"
>            extends="chrome://global/content/bindings/button.xml#button-base">
>     <resources>
>       <stylesheet src="chrome://global/skin/button.css"/>
>     </resources>
> 
>     <content>
>       <children includes="observes|template|menupopup|panel|tooltip"/>
>       <xul:hbox class="box-inherit button-box" xbl:inherits="align,dir,pack,orient"
>                 align="center" pack="center" flex="1" anonid="button-box">
>         <children>
>           <xul:image class="button-icon" xbl:inherits="src=image"/>
>           <xul:label class="button-text" xbl:inherits="value=label,accesskey,crop,highlightable"/>
>           <xul:label class="button-highlightable-text" xbl:inherits="xbl:text=label,accesskey,crop,highlightable"/>
>         </children>
>       </xul:hbox>
>     </content>
>   </binding>

Any binding that extends chrome://global/content/bindings/button.xml#button will need to have the new highlightable label added to its content. Then each xul:button that we want to make highlightable, we will need to set the attribute `highlightable="true"` on it.

We will then need to update toolkit/content/xul.css to have the following rules:

> .button-highlightable-text:not([highlightable="true"]),
> .button-text[highlightable="true"] {
>   display: none;
> }

This is all similar to what was done to make toolbar buttons with multiline labels work. See http://searchfox.org/mozilla-central/search?q=toolbarbutton-multiline-text

Comment 6

[Zachary Herrick]

Attachment: Bug 1340643 - Making some labels and all buttons highlightable.

Review commit: https://reviewboard.mozilla.org/r/125114/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125114/

Comment 7

[Zachary Herrick]

Comment on attachment 8852989 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125114/diff/1-2/

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8852989 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

https://reviewboard.mozilla.org/r/125114/#review127666

Looks good! Looking forward to the rest of the patch and you hooking up the buttons to use highlightable="true".

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8852989 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

https://reviewboard.mozilla.org/r/125114/#review129650

Comment 10

[Zachary Herrick]

Comment on attachment 8852989 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125114/diff/2-3/

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8852989 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

https://reviewboard.mozilla.org/r/125114/#review129654

::: toolkit/content/widgets/button.xml:230
(Diff revision 3)
>        <children includes="observes|template|menupopup|panel|tooltip"/>
>        <xul:hbox class="box-inherit button-box" xbl:inherits="align,dir,pack,orient"
>                  align="center" pack="center" flex="1" anonid="button-box">
>          <children>
>            <xul:image class="button-icon" xbl:inherits="src=image"/>
>            <xul:label class="button-text" xbl:inherits="value=label,accesskey,crop"/>

This label needs to inherit the highlightable attribute too.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8852989 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

https://reviewboard.mozilla.org/r/125114/#review131514

::: browser/components/preferences/in-content/containers.xul:21
(Diff revision 3)
>  </preferences>
>  
>  <hbox hidden="true"
>        class="container-header-links"
>        data-category="paneContainers">
> -  <label class="text-link" id="backContainersLink" value="&backLink.label;" />
> +  <label class="text-link" id="backContainersLink" >&backLink.label;</label>

Nit - remove extra space before >. Same goes for other places like this where the's a space before the closing > brace.

Comment 13

[Zachary Herrick]

Comment on attachment 8852989 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125114/diff/3-4/

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8852989 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

https://reviewboard.mozilla.org/r/125114/#review132240

Looks good, just these two minor issues below and then we can land this.

::: browser/components/preferences/in-content/containers.xul:21
(Diff revision 4)
>  </preferences>
>  
>  <hbox hidden="true"
>        class="container-header-links"
>        data-category="paneContainers">
> -  <label class="text-link" id="backContainersLink" value="&backLink.label;" />
> +  <label class="text-link" id="backContainersLink" >&backLink.label;</label>

nit, remove the space before the >

::: browser/components/preferences/in-content/main.xul:358
(Diff revision 4)
>    </hbox>
>  
>    <separator class="thin"/>
>  
>    <hbox id="addEnginesBox" pack="start">
> -    <label id="addEngines" class="text-link" value="&addMoreSearchEngines.label;"/>
> +    <label id="addEngines" class="text-link" >&addMoreSearchEngines.label;</label>

nit, remove the space before the >

Comment 15

[Zachary Herrick]

Comment on attachment 8852989 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125114/diff/4-5/

Comment 16

[Zachary Herrick]

Comment on attachment 8852989 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125114/diff/5-6/

Comment 17

[Manotej Meka (:manotejmeka)]

Attachment: Bug 1340643 - Making some labels and all buttons highlightable.

Review commit: https://reviewboard.mozilla.org/r/129756/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129756/

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8857781 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

Clearing review and marking as obsolete since Zack's attachment here is what we want. I pushed Zack's patch to autoland.

Comment 19

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s f9890eda5a90 -d 415a47617632: rebasing 389859:f9890eda5a90 "Bug 1340643 - Making some labels and all buttons highlightable. r=jaws" (tip)
merging browser/components/preferences/in-content/main.xul
merging browser/components/preferences/in-content/privacy.xul
warning: conflicts while merging browser/components/preferences/in-content/privacy.xul! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Hey Zack, can you rebase your patch here?

Comment 21

[Zachary Herrick]

Comment on attachment 8852989 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125114/diff/6-7/

Comment 22

[Zachary Herrick]

Comment on attachment 8852989 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125114/diff/7-8/

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8852989 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

Flagging Neil for review of the xul.css change.

Comment 24

[Neil Deakin]

Comment on attachment 8852989 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

This seems ok. Are we concerned performance-wise about the extra label being present in every button?

Comment 25

[Evan Tseng [:evanxd]]

Hi Jared,

Could we land the patch since we've already r+(ed) the it?

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Evan Tseng [:evanxd] from comment #25)
> Hi Jared,
> 
> Could we land the patch since we've already r+(ed) the it?

The patch will need to be rebased, and we may want to see what the performance impacts are based on comment #24. @Evan, can you rebase the patch and push it to tryserver with the following tryserver syntax?

./mach try -b o -p linux,win32,macosx64 -u mochitests -t all --rebuild-talos 5

Then do a push to tryserver without the patch but using the same syntax.

With those two pushes, we can compare the talos numbers to see if this will cause any performance regressions. If it does, then we may need to create a new binding that will be used only by the buttons within the preferences. Right now the current binding will affect buttons seen elsewhere.

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Hey Zack - I'm going to unassign you from this one. A full-timer will drive this one through to completion. Thanks for your work!

Comment 28

[Mike Conley (:mconley) (:⚙️)]

ni? to evanxd for comment 26.

Comment 29

[Evan Tseng [:evanxd]]

(In reply to Mike Conley (:mconley) from comment #28)
> ni? to evanxd for comment 26.

Thanks for the reminder, Mike.

(In reply to (Away 3 May - 5 May) Jared Wein [:jaws] (please needinfo? me) from comment #26)
> (In reply to Evan Tseng [:evanxd] from comment #25)
> > Hi Jared,
> > 
> > Could we land the patch since we've already r+(ed) the it?
> 
> The patch will need to be rebased, and we may want to see what the
> performance impacts are based on comment #24. @Evan, can you rebase the
> patch and push it to tryserver with the following tryserver syntax?
> 
> ./mach try -b o -p linux,win32,macosx64 -u mochitests -t all --rebuild-talos
> 5
> 
> Then do a push to tryserver without the patch but using the same syntax.
> 
> With those two pushes, we can compare the talos numbers to see if this will
> cause any performance regressions. If it does, then we may need to create a
> new binding that will be used only by the buttons within the preferences.
> Right now the current binding will affect buttons seen elsewhere.

Sure, I've rebased it[1]. And the belows are the talos tasks.

Without patch: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=a010f352e740b943053199ec00c2a09eb9dab439
With patch: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=2c7d77d7a42b932e794824240ea9c49068c4461d

[1]: https://hg.mozilla.org/try/rev/4fcf7f4c0d99f3e718fc15e89ed650562fc48031

Comment 30

[Evan Tseng [:evanxd]]

Waiting for the talos result.

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

Correct talos compare link: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a010f352e740b943053199ec00c2a09eb9dab439&newProject=try&newRevision=2c7d77d7a42b932e794824240ea9c49068c4461d&framework=1&showOnlyImportant=0

This looks good to land once it is rebased.

Comment 32

[Evan Tseng [:evanxd]]

OK, let me do the rebase.

Comment 33

[Evan Tseng [:evanxd]]

Attachment: Bug 1340643 - Making some labels and all buttons highlightable.

Review commit: https://reviewboard.mozilla.org/r/136082/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136082/

Comment 34

[Evan Tseng [:evanxd]]

Hi Mike,

I rebased the patch[1]. And the performance is good (Comment 31).

Could you take a look?

Thanks.

[1]: https://reviewboard.mozilla.org/r/125114

Comment 35

[Evan Tseng [:evanxd]]

Comment on attachment 8864396 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136082/diff/1-2/

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8864396 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

https://reviewboard.mozilla.org/r/136082/#review139728

Thanks!

Comment 37

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8864396 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

https://reviewboard.mozilla.org/r/136082/#review139810

::: commit-message-42f89:1
(Diff revision 2)
> +Bug 1340643 - Making some labels and all buttons highlightable.

Please attribute to the original author if your contribution did not go beyond rebase. Thank you.

Comment 38

[Evan Tseng [:evanxd]]

Comment on attachment 8864396 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136082/diff/2-3/

Comment 39

[Evan Tseng [:evanxd]]

Comment on attachment 8864396 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

https://reviewboard.mozilla.org/r/136082/#review139810

> Please attribute to the original author if your contribution did not go beyond rebase. Thank you.

Yes, let's update author info.

Comment 40

[Evan Tseng [:evanxd]]

Updated author and got r+. Let's ensure the treeherder[1] is good then we could land the code.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8a5a28177bf

Comment 41

[Evan Tseng [:evanxd]]

Comment on attachment 8864396 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136082/diff/3-4/

Comment 42

[Evan Tseng [:evanxd]]

Fixed the eslint failure.

Comment 43

[Evan Tseng [:evanxd]]

The treeherder[1] is good. Let's land the code.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7b19b3a2403&selectedJob=97345291

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc2cef40a9c1
Make some labels and all buttons highlightable. r=mconley

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dc2cef40a9c1

Comment 46

[Hani Yacoub, Desktop QA]

Build ID: 20170530030204
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

Verified as fixed on Firefox Nightly 55.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.