384612 - Get rid of script in scrollbar XBL binding

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We should use CSS to hide the needed parts of the scrollbar rather than using scripts to do it. Executing scrips at the current point causes all sorts of issues, and probably is a performance loss too.

My plan is to add rules like:

scrollbarbutton[sbattr="scrollbar-up-top"]:-moz-uielement-hidden(scrollbarUpTop)
{
  display: none;
}

The exact name of the CSS stuff is something i'd love input on.

Comment 1

[Colin Barrett [:cbarrett]]

You might want to take a look at bug 377439 as well.

Comment 2

[Martijn Wargers (dead)]

It would be nice if those pseudo css rules could be done on the scrollbar, so scrollbar:-moz-uielement-hidden, etc..
Also, I would like to have a scrollbar:disabled, instead of to have to use a scrollbar[disabled].

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why is it preferable to have it on the scrollbar rather than the scrollbarbutton? And exactly what syntax are you proposing, your above comment just includes the selector.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

Ideally we should store pseudo class arguments as atoms rather than strings, but david asked me to do that as a different bug. (which probably won't happen for this release unless the relevant code shows up in profiles)

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Roc, if you want to review the patch feel free to.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I would, but there are style system changes that I'd rather not know about :-).

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 287740 [details] [diff] [review]
Patch to fix

Can you think of any harm in exposing this to content?  I haven't yet, but it's worth thinking about.

I'd prefer if the *initialization* in nsCSSRuleProcessor were lazy; we don't want to create things at startup until we need to.

:-moz-look-and-feel-metric-set is awfully wordy.  I think the "-set" is sort of implied by the way CSS Selectors work.  Maybe :-moz-system-metric would be better?  Although I'm not a big fan of "metric" either...

The "-hidden" names for the metrics are also pretty confusing.  Any chance you could drop them and reverse the selectors, so that the buttons default to display:none and have selectors that make them show up?

CSSRuleProcessor::Shutdown should assign null to the variable it's deleting.

+               sSetLookAndFeelMetrics->NoIndex;

Does that work portably, or do you need the name of the type?

r+sr=dbaron with those fixed

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #7)
> (From update of attachment 287740 [details] [diff] [review])
> Can you think of any harm in exposing this to content?  I haven't yet, but 
> it's worth thinking about.

I can't think of any real harm, but I would like to avoid it. The problem is that I can't think of any way to. The problem is that even if limit it to chrome stylesheets, xul.css will still be loaded and you could simply put an <scrollbarbutton sbattr="scrollbar-up-top"> in your DOM and test if it's visible.

So basically I can't think of any way to fix it that isn't really complicated, and I don't think it's worth it given that this isn't that important information being leaked.

(and technically, this isn't a regression, you could get at this info from the boxobject before this patch)

> :-moz-look-and-feel-metric-set is awfully wordy.  I think the "-set" is sort 
> of implied by the way CSS Selectors work.  Maybe :-moz-system-metric would be
> better?  Although I'm not a big fan of "metric" either...

I'm no fan of "metric" either, but it's what we use everywhere else. I'll do -moz-system-metric though.

> The "-hidden" names for the metrics are also pretty confusing.  Any chance you
> could drop them and reverse the selectors, so that the buttons default to
> display:none and have selectors that make them show up?

Hmm.. Is the added cleanness to the names worth the duplicated stylerules, and the perf-hit that comes with that? These names won't be used anywhere else I'm sure.

Up to you.

> CSSRuleProcessor::Shutdown should assign null to the variable it's deleting.
> 
> +               sSetLookAndFeelMetrics->NoIndex;
> 
> Does that work portably, or do you need the name of the type?

Should work fine. We do foo.NoIndex and foo->NoIndex in a bunch of other places (in fact it's common way its used).

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #8)
> > The "-hidden" names for the metrics are also pretty confusing.  Any chance you
> > could drop them and reverse the selectors, so that the buttons default to
> > display:none and have selectors that make them show up?
> 
> Hmm.. Is the added cleanness to the names worth the duplicated stylerules, and
> the perf-hit that comes with that? These names won't be used anywhere else I'm
> sure.
> 
> Up to you.

I think it is.  And, in fact, you can just use :not(), and not need any more style rules.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v2

Excellent idea to use :not()

This patch should do it. I'll check it in monday.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Fix checked in. Thanks for the review!

Comment 12

[[e-mail]]

Hey, since scrollbars and tooltips no longer need to run scripts, does this mean that we can remove the IsNativeAnonymous override in nsXULElement?

Comment 13

[Boris Zbarsky [:bzbarsky]]

I think we could, yes!  File a bug to do that, and to eliminate all the GetBindingParent() hackery we have instead in some places?  It'd be lovely to get that done for 1.9...  For one thing, it would close up some potential security issues.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hey, sweet, I didn't realize tooltips were fixed too. Please do file a bug and cc me and bz at least.