Closed Bug 1340643 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: UI Widgets, defect, P1)

Product:
Toolkit
Component:
UI Widgets
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- verified

People

(Reporter: mconley, Assigned: evanxd)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference] )

Attachments

(1 file, 2 obsolete files)

Bug 1340643 - Making some labels and all buttons highlightable.
59 bytes, text/x-review-board-request
mconley
: review+
Details 
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)
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 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 on attachment 8852989 [details]
Bug 1340643 - Making some labels and all buttons highlightable.

https://reviewboard.mozilla.org/r/125114/#review129650
Attachment #8852989 - Flags: 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 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 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)
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)
Blocks: 1357285
Whiteboard: [photon-preference]
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 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+
Target Milestone: --- → mozilla55
QA Contact: hani.yacoub
Flags: qe-verify+
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
(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)
Waiting for the talos result.
OK, let me do the rebase.
Assignee: nobody → evan
Status: NEW → ASSIGNED
Attachment #8864396 - Flags: review?(mconley)
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
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 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.
Attachment #8852989 - Attachment is obsolete: true
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
Fixed the eslint failure.
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
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
https://hg.mozilla.org/mozilla-central/rev/dc2cef40a9c1
Status: ASSIGNED → RESOLVED
Closed: 7 years 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
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.

Attachment

General

Creator:
Created:
Updated:
Size: