Closed Bug 1308286 Opened 3 years ago Closed 2 months ago

Scroll less to give more context with page down/page up

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox52 --- wontfix
firefox71 --- fixed

People

(Reporter: overholt, Assigned: tnikkel, NeedInfo)

References

Details

Attachments

(1 file, 1 obsolete file)

Looks like we tried to improve things in bug 780345 but if I go to, say, https://www.thestar.com/sports/bluejays/2016/10/06/its-go-time-jays-rangers-game-1-alds-preview.html and press Page Down a few times after setting text at the top, we scroll too much and I miss lines of text. See attached video.

1. Navigate to https://www.thestar.com/sports/bluejays/2016/10/06/its-go-time-jays-rangers-game-1-alds-preview.html
2. Scroll down with the scrollwheel or scrollbar to have "Pompey left off ALDS roster" just visible at the top under thestar.com's banner thing
3. Press Page Down

Expected:

A bit of overlap of what text is visible so I can continue reading from where I was.

Actual:

A line I didn't read has been entirely scrolled off (up; behind the banner thing).
Ideally I'd like to see a setting exposed inside about:config that allows you to specify a percentage of the page to scroll down by with each page_down or space bar press.
we should probably (a) scroll a bit less to match other browsers (whether or not there is some banner) and (b) figure out why bug 780345 doesn't seem to work the expected way with https://www.thestar.com

tn, you reviewed Bug 780345, could you perhaps take a look?
Flags: needinfo?(tnikkel)
tn looked at bug 1225201 which I think may be the same issue and said nothing stuck out for him there.
See Also: → 1225201
There is an obvious reason for this testcase: the fixed header is sticky position. The existing code only considers fixed position headers; there isn't any logic implemented to take into account sticky position headers at the moment.

Sticky positioning adds some complications: what happens if the sticky position item would become/stop being sticky during the potential scroll we are computing how far to scroll for? We might be able to ignore this complication and just compute the value looking at what is sticky right then. The edge cases might not be too important or come up much.
Attached patch quick hack (obsolete) — Splinter Review
Here is a quick hack that seems to fix it. I'm not sure if there's a better way to detect if sticky frames that are acting like fixed.
Comment on attachment 8837029 [details] [diff] [review]
quick hack

Botond, you last touched the sticky container code, perhaps you understand it better than I. Is this patch sane?
Attachment #8837029 - Flags: feedback?(botond)
Comment on attachment 8837029 [details] [diff] [review]
quick hack

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

Yep, this looks reasonable to me.

::: layout/generic/StickyScrollContainer.h
@@ +87,5 @@
>    virtual void ScrollPositionDidChange(nscoord aX, nscoord aY) override;
>  
>    ~StickyScrollContainer();
>  
> +  nsTArray<nsIFrame*>& GetFrames() { return mFrames; }

Return |const nsTArray<nsIFrame*>&| here, that way the caller cannot modify the array.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +4149,5 @@
> +    nsTArray<nsIFrame*>& stickyFrames = ssc->GetFrames();
> +    for (nsIFrame* f : stickyFrames) {
> +      // if its acting like fixed position
> +      if (ssc->IsStuckInYDirection(f)) {
> +        nsRect r = f->GetRectRelativeToSelf();

There's probably opportunity to factor out a helper betwen this loop and the loop above.
Attachment #8837029 - Flags: feedback?(botond) → feedback+
Too late for firefox 52, mass-wontfix.
Duplicate of this bug: 1587144
Attachment #8837029 - Attachment is obsolete: true
Flags: needinfo?(tnikkel)
Attachment #9099723 - Attachment description: Bug 1308286. Consider sticky frames that are acting like fixed position frames when determining page scroll amount. r?botond → Bug 1308286. Consider sticky frames that are acting like fixed position frames when determining page scroll amount. r=botond
Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/263936aecc1d
Consider sticky frames that are acting like fixed position frames when determining page scroll amount. r=botond
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Assignee: nobody → tnikkel
Flags: qe-verify+

I did not manage to reproduce the issue following the STR provided in description on a build prior to the fix (Fx 71.0a1 2019-10-06). Can you please verify that the issue is fixed using the latest Fx http://archive.mozilla.org/pub/devedition/candidates/71.0b11-candidates/build1/

Flags: needinfo?(tnikkel)
You need to log in before you can comment on or make changes to this bug.