1082098 - Unable to dynamically change scroll-behavior property

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[:kip (Kearwood Gilbert)]

Dynamic changes to the scroll-behavior property on html/body after pageload don't have an effect on whether anchor scrolling is smooth.

Comment 1

[:kip (Kearwood Gilbert)]

Please see Bug 1010538 Comment 103

Comment 2

[:kip (Kearwood Gilbert)]

Attachment: Bug 1082098 - Return correct change hint when scroll-behavior CSS value changes

- When the scroll-behavior CSS value changed, the nsChangeHint_NeutralChange
hint was returned by nsStyleDisplay::CalcDifference. It now returns
nsChangeHint_ReconstructFrame to ensure that the change takes effect.

A part2 patch will be added with a test.

Comment 3

[:kip (Kearwood Gilbert)]

Attachment: Bug 1082098: Part 2 - Tests

- scroll-behavior-8.html - Tests if dynamically changing the scroll-behavior
css property on a div element takes effect after the page has been painted and
reflowed.
- scroll-behavior-9.html - Tests if dynamically changing the scroll-behavior
on the body element takes effect after the page has been painted and
reflowed.

Comment 4

[:kip (Kearwood Gilbert)]

Attachment: Bug 1082098: Part 1 - Return correct change hint when scroll-behavior CSS value changes (V2 Patch)

- When the scroll-behavior CSS value changed, the nsChangeHint_NeutralChange
hint was returned by nsStyleDisplay::CalcDifference. It now returns
nsChangeHint_ReconstructFrame to ensure that the change takes effect.

V2 Patch:
- Adjusted commit message

Comment 5

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

Comment on attachment 8510730 [details] [diff] [review]
Bug 1082098: Part 1 - Return correct change hint when scroll-behavior CSS value changes (V2 Patch)

Review of attachment 8510730 [details] [diff] [review]:
-----------------------------------------------------------------

This is OK, but I don't understand why it's needed. Can you explain?

Comment 6

[:kip (Kearwood Gilbert)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> Comment on attachment 8510730 [details] [diff] [review]
> Bug 1082098: Part 1 - Return correct change hint when scroll-behavior CSS
> value changes (V2 Patch)
> 
> Review of attachment 8510730 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is OK, but I don't understand why it's needed. Can you explain?
When scroll-behavior is changed, the nsChangeHint_NeutralChange was not sufficient to enter nsCSSFrameConstructor::PropagateScrollToViewport.  By using the same hint as used when the overflow css property changes, nsChangeHint_ReconstructFrame, PropagateScrollToViewport will be called.

The scroll-behavior css property is not expected to change often (the CSSOM-View DOM methods are likely to be used in those cases); however, if this does become common perhaps a faster-path might be worth while.

Comment 7

[:kip (Kearwood Gilbert)]

Pushed to try:

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

Comment 8

[:kip (Kearwood Gilbert)]

(In reply to :kip (Kearwood Gilbert) from comment #7)
> Pushed to try:
> 
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ca029b711a26
Try is green...

Comment 9

[:kip (Kearwood Gilbert)]

(In reply to :kip (Kearwood Gilbert) from comment #6)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> > Comment on attachment 8510730 [details] [diff] [review]
> > Bug 1082098: Part 1 - Return correct change hint when scroll-behavior CSS
> > value changes (V2 Patch)
> > 
> > Review of attachment 8510730 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This is OK, but I don't understand why it's needed. Can you explain?
> When scroll-behavior is changed, the nsChangeHint_NeutralChange was not
> sufficient to enter nsCSSFrameConstructor::PropagateScrollToViewport.  By
> using the same hint as used when the overflow css property changes,
> nsChangeHint_ReconstructFrame, PropagateScrollToViewport will be called.
> 
> The scroll-behavior css property is not expected to change often (the
> CSSOM-View DOM methods are likely to be used in those cases); however, if
> this does become common perhaps a faster-path might be worth while.
Missed the needinfo flag, adding it.

Comment 10

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

Comment on attachment 8510730 [details] [diff] [review]
Bug 1082098: Part 1 - Return correct change hint when scroll-behavior CSS value changes (V2 Patch)

Review of attachment 8510730 [details] [diff] [review]:
-----------------------------------------------------------------

Please add a comment with your explanation of why this is needed.

Comment 11

[:kip (Kearwood Gilbert)]

Attachment: Bug 1082098: Part 1 - Return correct change hint when scroll-behavior CSS value changes (V3 Patch), r=roc

- When the scroll-behavior CSS value changed, the nsChangeHint_NeutralChange
hint was returned by nsStyleDisplay::CalcDifference. It now returns
nsChangeHint_ReconstructFrame to ensure that the change takes effect.
- When scroll-behavior is changed, the nsChangeHint_NeutralChange was not
sufficient to enter nsCSSFrameConstructor::PropagateScrollToViewport. By
using the same hint as used when the overflow css property changes,
nsChangeHint_ReconstructFrame, PropagateScrollToViewport will be called.
- The scroll-behavior css property is not expected to change often (the
CSSOM-View DOM methods are likely to be used in those cases); however, if
this does become common perhaps a faster-path might be worth while.

V3 Patch
- Added comment to commit message and code documenting why the nsChangeHint_ReconstructFrame is returned for scroll-behavior changes.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cd46e671a9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/af957b6bf421

Comment 13

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3389325&repo=mozilla-inbound

Comment 14

[:kip (Kearwood Gilbert)]

Attachment: Bug 1082098: Part 2 - Tests (V2 Patch)

- scroll-behavior-8.html - Tests if dynamically changing the scroll-behavior
  css property on a div element takes effect after the page has been painted and
  reflowed.
- scroll-behavior-9.html - Tests if dynamically changing the scroll-behavior
  on the body element takes effect after the page has been painted and
  reflowed.

V2 Patch:

- Increased the size of the scroll boxes to correct reftest failures on Windows XP.  Reftest failures were due to inconsistent rendering of scrollbar drag handles when size of scroll box was exactly at the previous size.

Comment 15

[:kip (Kearwood Gilbert)]

Pushed to try to re-test the reftest that had failed on Windows XP:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=03651297787e

(Came back green)

Comment 16

[:kip (Kearwood Gilbert)]

Comment on attachment 8513830 [details] [diff] [review]
Bug 1082098: Part 2 - Tests (V2 Patch)

Increased the size of the scroll boxes to accommodate rendering of Windows XP scroll bar drag handles.  Need to review?

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/07daf18bf013
https://hg.mozilla.org/integration/mozilla-inbound/rev/c13d91ae9f04

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/07daf18bf013
https://hg.mozilla.org/mozilla-central/rev/c13d91ae9f04