1431522 - Remove the 'thumb' binding

Status: RESOLVED FIXED

(Core :: XUL, task)

Description

[Brian Grinstead [:bgrins]]

From https://bugzilla.mozilla.org/show_bug.cgi?id=1431246#c2: 

> Neil, is it possible we could remove the "thumb" binding at
> https://searchfox.org/mozilla-central/source/toolkit/content/widgets/
> scrollbar.xml#12? It's bound to the "thumb" element at
> https://searchfox.org/mozilla-central/source/toolkit/content/minimal-xul.
> css#91. I'm not sure exactly what `extends="xul:button"` is doing - is this
> something we could delete and/or move into XUL <thumb> directly instead of
> through XBL?

And https://bugzilla.mozilla.org/show_bug.cgi?id=1431246#c3: 

> That line causes the element to get a nsButtonBoxFrame instead of nsBoxFrame.
> 
> But you should be able to easily remove the need for a binding by adding a
> mapping for <thumb> in nsCSSFrameConstructor::FindXULTagData, and also
> adding to the whitelist at nsXBLPrototypeBinding::CheckTagNameWhiteList

Comment 1

[Brian Grinstead [:bgrins]]

Neil, does the `display: -moz-box !important` on thumb https://dxr.mozilla.org/mozilla-central/source/toolkit/content/minimal-xul.css#92 remove the need to set up the mapping for nsButtonBoxFrame in nsCSSFrameConstructor::FindXULTagData?

Comment 2

[Neil Deakin]

No, that just gives it a box frame, which is generally redundant since it should already be one. It looks like the display line was added by 248407 to fix a crash.

Comment 3

[Brian Grinstead [:bgrins]]

> But you should be able to easily remove the need for a binding by adding a
> mapping for <thumb> in nsCSSFrameConstructor::FindXULTagData, and also
> adding to the whitelist at nsXBLPrototypeBinding::CheckTagNameWhiteList

I've looked at nsXBLPrototypeBinding::CheckTagNameWhiteList and it's not clear to me why we would need to whitelist thumb here. That appears to be used for the 'display' or 'extends' attributes on the binding, and I don't see anything that uses display='xul:thumb' or extends='xul:thumb' (https://dxr.mozilla.org/mozilla-central/search?q=display%3D%22xul&redirect=true / https://dxr.mozilla.org/mozilla-central/search?q=extends%3D%22xul&redirect=false).

The only non-tag hit for "xul:thumb" has to do with accesibility roles, which shouldn't be affected by that: https://dxr.mozilla.org/mozilla-central/search?q=xul%3Athumb&redirect=true.  It's a bit odd that the thumb itself doesn't have that role, but doesn't seem like removing the thumb binding would change anything there.

Comment 4

[Neil Deakin]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> > But you should be able to easily remove the need for a binding by adding a
> > mapping for <thumb> in nsCSSFrameConstructor::FindXULTagData, and also
> > adding to the whitelist at nsXBLPrototypeBinding::CheckTagNameWhiteList
> 

Yes, I realize that you don't need to add it in this case. Adding to FindXULTagData should suffice.

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: Bug 1431522 - Remove the 'thumb' binding;+6102

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

Comment 6

[Brian Grinstead [:bgrins]]

I'm not sure if the change to FindXULTagData is right - I just copied what was there for `button`.

Comment 7

[Brian Grinstead [:bgrins]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=34780b432a812cd8418ba312f27489ea22bb8c43

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8943979 [details]
Bug 1431522 - Remove the 'thumb' binding;+6102

Somehow the reviewer field didn't get set in mozreview

Comment 9

[Neil Deakin]

Comment on attachment 8943979 [details]
Bug 1431522 - Remove the 'thumb' binding;+6102

https://reviewboard.mozilla.org/r/214304/#review229552

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8943979 [details]
Bug 1431522 - Remove the 'thumb' binding;+6102

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

Comment 11

[Brian Grinstead [:bgrins]]

Rebased - review flag somehow didn't get carried over but I think mozreview still sees it as an r+ since the land commits button is active.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee37791e04a98f9d7f87f2b435817601be970bd4

Comment 12

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

I am pretty sure when this lands the scalethumb binding can be removed as well (in bug 1440393).

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #12)
> I am pretty sure when this lands the scalethumb binding can be removed as
> well (in bug 1440393).

Yeah - although it looks like we'll also probably need to import scale.css into components.css and remove it out of all <resources> in scale.xml.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a20647d8a887
Remove the 'thumb' binding;r=enndeakin+6102+6102

Comment 15

[Noemi Erli[:noemi_erli]]

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