1321769 - stylo: scrollbars don't work

Status: RESOLVED WORKSFORME

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Cameron McCormack (:heycam)]

On Linux at least, scrollbars don't work in Servo-styled documents.  They appear, but the thumb doesn't move or get resized appropriately.

Comment 1

[Bobby Holley (:bholley)]

Was this fixed by bug 132128, or is there still more to do here?

Comment 2

[Cameron McCormack (:heycam)]

No, scroll bars aren't visible at all now in Linux.

Comment 3

[Bobby Holley (:bholley)]

A testcase from bz in bug 1341095:

data:text/html,<div style="width: 200%; height: 20px; background: blue"></div>

It sounds like this is not restricted to linux.

Comment 5

[Bobby Holley (:bholley)]

Ting-Yu, can you take a look at this one?

Comment 6

[Bobby Holley (:bholley)]

Ah, bug 1321754 comment 0 indicates that that bug is the cause of this one. Assigning to Ting-yu.

Comment 7

[Shing Lyu [:shinglyu]]

Attachment: related reftest failures

Here are the list of reftest failures caused by this.

Comment 8

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

[1] is the stylo vs gecko frame tree dump for the test case in comment 3.

In stylo, no SliderFrame and ButtonBoxFrame is generated under ScrollbarFrame.

We can simply change the SheetParsingMode of scrollbars.css from eAuthorSheetFeatures to eAgentSheetFeatures in [2] to make SliderFrame and ButtonBoxFrame generated in stylo.

With above, the scrollbar is visible [3] but it is still malfunction. It cannot be dragged and its position relative to the document is wrong. Perhaps some properties are in [4] are still not supported.

[1] https://pastebin.mozilla.org/8983289
[2] http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/layout/style/nsLayoutStylesheetCache.cpp#78-79 
[3] On Mac, you need to change Settings -> General -> Show scroll bars -> Always. Some dynamic feature is missing in stylo...
[4] http://searchfox.org/mozilla-central/source/toolkit/themes/osx/global/nativescrollbars.css

Comment 9

[Boris Zbarsky [:bzbarsky]]

> We can simply change the SheetParsingMode of scrollbars.css from eAuthorSheetFeatures to eAgentSheetFeatures 

We can, but how about we fix bug 1321754 instead of increasing our security exposure?

Comment 10

[Bobby Holley (:bholley)]

How much work is there to do after bug 1321754 to get scrollbars working? If it's significant, it might make sense to parallelize it given that so many tests are failing due to scrollbar issues.

Jeremy has some cycles to work on high-priority stuff, so let us know if you think it would make sense to have him work on some of this.

Comment 11

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Recently, after applying my patch in bug 1321754, the scrollbar is working at least on Linux and Mac. The issues I described in comment 8 no longer exist. Dragging the scrollbar and the position is working now.

I don't think there's much to do after bug 1321754 lands, but I'll push a try to see whether there's anything significant left.

Comment 13

[Bobby Holley (:bholley)]

Manish, did bug 1321754 fix the scrollbar issues you were seeing?

Comment 14

[Manish Goregaokar [:manishearth]]

Nope.

https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/csLA_yz8TuaE4eAlKphjyw/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

(See first few tests, e.g. layout/reftests/reftest-sanity/656041-1.html)


layout/reftests/invalidation/transform-floating-point-invalidation.html in https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/TIkFStnWTYisYAyOFRJ-Ow/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1



The scrollbar failures still seem to exist everywhere.

Comment 15

[Bobby Holley (:bholley)]

Ok, NI TY and Hiro to figure out what's going on here.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

The difference is light gray bars inside scrollbars. I guess it's caused by lack of -moz-appearance support (bug 1349651).

Comment 17

[Bobby Holley (:bholley)]

Manish, can you hack up a patch to change the line at [1] to |return false;| and re-do your try push from comment 14? It would be good to know if most/all of those scrollbar failures will be fixed by HASR, or whether there are other things we should be working on in parallel.

[1] http://searchfox.org/mozilla-central/rev/abe68d5dad139e376d5521ca1d4b7892e1e7f1ba/layout/style/nsRuleNode.cpp#10500

Comment 18

[Manish Goregaokar [:manishearth]]

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac8e58ffc537665afe44714ac751dd7d77ffc547 is the same thing without the test expectations all marked as passing (so we can see the potential impact of this change)

Comment 19

[Manish Goregaokar [:manishearth]]

Nope. layout/reftests/reftest-sanity/656041-1.html still fails there, and most of the failures are still scrollbar failures.

Hiro, is there something else we may have missed?

Comment 20

[Manish Goregaokar [:manishearth]]

Going to try a push which breaks both gecko and stylo the same way, to remove all effects of HasAuthorSpecifiedRules on the failures.

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

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Here is a try with tweaks that -moz-appearance in vertical scrollbar styles is removed.

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

(layout/reftests/reftest-sanity/656041-1.html passed there)

So, I still believe -moz-appearance is needed for those reftests.

Oh, I just realized that scrollbar-horizontal and scrollbar-vertical are not listed in supported -moz-appearance keywords.

https://hg.mozilla.org/mozilla-central/file/a748acbebbde/servo/components/style/properties/longhand/box.mako.rs#l2404

Comment 22

[Hiroyuki Ikezoe (:hiro)]

FWIW, a list that properties in nsCSSProps::kMozAppearanceKTable but not in -moz-appearance keywords for stylo.

-moz-gtk-info-bar
-moz-mac-active-source-list-selection
-moz-mac-disclosure-button-closed
-moz-mac-disclosure-button-open
-moz-mac-fullscreen-button
-moz-mac-help-button
-moz-mac-source-list
-moz-mac-source-list-selection
-moz-mac-vibrancy-dark
-moz-mac-vibrancy-light
dialog
scrollbar
scrollbar-horizontal
scrollbar-small
scrollbar-vertical
window

Comment 23

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #22)
> FWIW, a list that properties in nsCSSProps::kMozAppearanceKTable but not in
> -moz-appearance keywords for stylo.

Filed bug 1361632 for this.

Comment 24

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

After bug 1361632 was landed, scrollbar works pretty well on my Mac, which does the dynamic hide and show. Let's keep bug open, and see what else do we need to fix after HasAuthorSpecifiedRules is implemented.

Comment 25

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

I don't see new issue related to scroll bar.