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

VERIFIED FIXED in Firefox 55

Status

()

Toolkit
XUL Widgets
P1
normal
VERIFIED FIXED
11 months ago
8 months ago

People

(Reporter: mconley, Assigned: evanxd)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox55 verified)

Details

(Whiteboard: [photon-preference] )

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

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.
Component: Preferences → XUL Widgets
Product: Firefox → Toolkit
Summary: Some text-bearing elements on the page do not expose text nodes which we can highlight for search queries → xul:button and xul:menulist should provide the ability to get access to the textNode that displays the resulting text
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.
xul:label's that use the value attribute instead of the textContent approach also do not have the textNode child.
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.
Flags: needinfo?(enndeakin)

Comment 4

10 months ago
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.
Flags: needinfo?(enndeakin)
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
Assignee: nobody → herrickz
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

10 months ago
mozreview-review
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".
Attachment #8852989 - Flags: review?(jaws)

Comment 9

10 months ago
mozreview-review
Comment on attachment 8852989 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

https://reviewboard.mozilla.org/r/125114/#review129650
Attachment #8852989 - Flags: review-
Attachment #8852989 - Flags: review?(mconley)
Comment hidden (mozreview-request)

Comment 11

10 months ago
mozreview-review
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.
Attachment #8852989 - Flags: review?(jaws) → review-
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.
Attachment #8852989 - Flags: review?(mconley) → review-
Comment hidden (mozreview-request)

Comment 14

9 months ago
mozreview-review
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 >
Attachment #8852989 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Attachment #8857781 - Attachment is obsolete: true
Attachment #8857781 - Flags: review?(mconley)
Attachment #8857781 - Flags: review?(jaws)

Comment 19

9 months ago
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)
Hey Zack, can you rebase your patch here?
Flags: needinfo?(herrickz)

Updated

9 months ago
Blocks: 1357285
Whiteboard: [photon-preference]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8852989 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

Flagging Neil for review of the xul.css change.
Attachment #8852989 - Flags: review?(enndeakin)

Comment 24

9 months ago
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?
Attachment #8852989 - Flags: review?(enndeakin) → review+

Updated

9 months ago
Target Milestone: --- → mozilla55

Updated

9 months ago
QA Contact: hani.yacoub

Updated

9 months ago
Flags: qe-verify+
(Assignee)

Comment 25

9 months ago
Hi Jared,

Could we land the patch since we've already r+(ed) the it?
Flags: needinfo?(jaws)
(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.
Flags: needinfo?(jaws)
Flags: needinfo?(herrickz)
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!
Assignee: herrickz → administration
Status: ASSIGNED → NEW
ni? to evanxd for comment 26.
Flags: needinfo?(evan)
Assignee: administration → nobody
(Assignee)

Comment 29

9 months ago
(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
Flags: needinfo?(evan)
(Assignee)

Comment 30

9 months ago
Waiting for the talos result.
(Assignee)

Comment 32

9 months ago
OK, let me do the rebase.
Assignee: nobody → evan
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8864396 - Flags: review?(mconley)
(Assignee)

Comment 34

9 months ago
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 hidden (mozreview-request)

Updated

9 months ago
Priority: -- → P1
Comment on attachment 8864396 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

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

Thanks!
Attachment #8864396 - Flags: review?(mconley) → review+
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 hidden (mozreview-request)
(Assignee)

Comment 39

9 months ago
mozreview-review-reply
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.
(Assignee)

Updated

9 months ago
Attachment #8852989 - Attachment is obsolete: true
(Assignee)

Comment 40

9 months ago
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 hidden (mozreview-request)
(Assignee)

Comment 42

9 months ago
Fixed the eslint failure.
(Assignee)

Comment 43

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

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

Comment 44

9 months ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc2cef40a9c1
Make some labels and all buttons highlightable. r=mconley
Keywords: checkin-needed

Comment 45

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dc2cef40a9c1
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Summary: xul:button and xul:menulist should provide the ability to get access to the textNode that displays the resulting text → xul:button provide the ability to get access to the textNode that displays the resulting text

Comment 46

8 months ago
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.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
You need to log in before you can comment on or make changes to this bug.