Closed Bug 1082098 Opened 7 years ago Closed 7 years ago

Unable to dynamically change scroll-behavior property

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: kip, Assigned: kip)

References

Details

Attachments

(2 files, 3 obsolete files)

Dynamic changes to the scroll-behavior property on html/body after pageload don't have an effect on whether anchor scrolling is smooth.
Please see Bug 1010538 Comment 103
- 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.
Attached patch Bug 1082098: Part 2 - Tests (obsolete) — Splinter Review
- 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.
Attachment #8510727 - Flags: review?(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.

V2 Patch:
- Adjusted commit message
Attachment #8510615 - Attachment is obsolete: true
Attachment #8510730 - Flags: review?(roc)
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?
Attachment #8510730 - Flags: review?(roc)
(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.
(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...
(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.
Flags: needinfo?(roc)
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.
Attachment #8510730 - Flags: review+
- 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.
Attachment #8510730 - Attachment is obsolete: true
Keywords: checkin-needed
- 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.
Attachment #8510727 - Attachment is obsolete: true
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 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?
Attachment #8513830 - Flags: review?(roc)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/07daf18bf013
https://hg.mozilla.org/mozilla-central/rev/c13d91ae9f04
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.