stylo: scrollbars don't work

RESOLVED WORKSFORME

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED WORKSFORME
8 months ago
a month ago

People

(Reporter: heycam, Assigned: TYLin)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

8 months ago
On Linux at least, scrollbars don't work in Servo-styled documents.  They appear, but the thumb doesn't move or get resized appropriately.
Was this fixed by bug 132128, or is there still more to do here?
Flags: needinfo?(cam)
(Reporter)

Comment 2

5 months ago
No, scroll bars aren't visible at all now in Linux.
Flags: needinfo?(cam)
Priority: -- → P2
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.
Blocks: 1324348
Duplicate of this bug: 1341095
Ting-Yu, can you take a look at this one?
Assignee: nobody → tlin
Priority: P2 → P1
Ah, bug 1321754 comment 0 indicates that that bug is the cause of this one. Assigning to Ting-yu.
(Assignee)

Updated

4 months ago
Status: NEW → ASSIGNED

Comment 7

4 months ago
Created attachment 8851451 [details]
related reftest failures

Here are the list of reftest failures caused by this.
(Assignee)

Comment 8

4 months ago
[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

4 months ago
> 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?
(Assignee)

Updated

4 months ago
Depends on: 1321754
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.
Flags: needinfo?(tlin)
(Assignee)

Comment 11

3 months ago
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.
Flags: needinfo?(tlin)

Updated

3 months ago
Blocks: 1351978
Duplicate of this bug: 1361045
Manish, did bug 1321754 fix the scrollbar issues you were seeing?
Flags: needinfo?(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.
Flags: needinfo?(manishearth)
Ok, NI TY and Hiro to figure out what's going on here.
Flags: needinfo?(tlin)
Flags: needinfo?(hikezoe)
The difference is light gray bars inside scrollbars. I guess it's caused by lack of -moz-appearance support (bug 1349651).
Depends on: 1349651
Flags: needinfo?(hikezoe)
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
Flags: needinfo?(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)
Flags: needinfo?(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?
Flags: needinfo?(hikezoe)
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
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
Flags: needinfo?(hikezoe)
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
(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.

Updated

3 months ago
Depends on: 1361632
(Assignee)

Comment 24

3 months ago
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.
Flags: needinfo?(tlin)

Updated

3 months ago
No longer blocks: 1351978
(Assignee)

Comment 25

a month ago
I don't see new issue related to scroll bar.
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.