WebExtension browser action popup scrolls to the top on changing an element's style

VERIFIED FIXED in Firefox 56

Status

P2
normal
VERIFIED FIXED
2 years ago
2 months ago

People

(Reporter: justin.mopp, Assigned: angelsl)

Tracking

({regression, testcase})

50 Branch
mozilla57
regression, testcase

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 verified, firefox57 verified)

Details

(Whiteboard: triaged)

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Created attachment 8819956 [details]
webextensions_scroll_bug-0.1.zip

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161209094039

Steps to reproduce:

I unzipped the attached web extension and loaded the manifest as a temporary add-on from about:debugging. I then clicked on the extension's icon, scrolled the popup down to around the middle, and clicked on a list item.


Actual results:

The list item's style changed - as expected, but the popup instantly scrolled to the top.


Expected results:

The popup should have kept its scroll position. I tested this in Firefox 49 and it worked as expected.

Updated

2 years ago
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit

Updated

2 years ago
Status: UNCONFIRMED → NEW
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Ever confirmed: true

Comment 1

2 years ago
filing another bug, someone from layout team planning on working on it.
Flags: needinfo?(kmaglione+bmo)
Priority: -- → P2

Comment 2

2 years ago
Was there a follow up bug on this? Or was Kris going to do that.
Flags: needinfo?(sescalante)
Whiteboard: triaged
Duplicate of this bug: 1350077

Comment 4

a year ago
I'm experiencing the same issue. I made a few tests and it seems any change in the DOM triggers the scroll to the top (not only changes that modify the height of the popup).

I found this issue in the process of converting my Chrome extension into a WebExtension. The problem doesn't occur on Chrome.

Comment 5

a year ago
uMatrix WebExtension suffers from this bug as well: hovering over the matrix cells will always reset the scroll position to the top. So if you have a long list of domains and want to set a rule for a row which is visible only if you scroll down, the matrix will jump back to the top.

This bug is not reproduced if I move uMatrix icon to the overflow menu(right click on uMatrix icon - Pin to Overflow Menu). However, there is no horizontal scrollbar and the width of the overflow menu is too small so the most part of the uMatrix dashboard is not visible. I don't know if that's intended or not.

Comment 6

a year ago
Rephrasing comment 5 a bit: this bug renders uMatrix/webex unusable, and since uMatrix non-webex was rendered unusable by recent nightly updates, there isn't a working version anymore. Plenty of people are waiting for this to be fixed.

Comment 7

a year ago
It's a big issue on our side too. The only thing I can do is to reduce the height of the popup the maximum I can but it's most of the time not really possible. It's really painful for the users.

Please let me know if I can help with more details about the issue and the steps to reproduce.

Comment 8

a year ago
Regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c9edfe35619f69f7785776ebd19a3140684024dc&tochange=939ecc4e9d055c263633cbe276bfb634a68fe4c5 
A patch change this popup to expanded, no scroll bar.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=adc8d8b7c96752a758a153f66ca5c32cf0f6c23b&tochange=cbe922c12cb08ab43cedb934459c554a4a0a64d6
Bug 1294442 change this popup to the original size, but the behavior has this bug.
Has Regression Range: --- → yes
Has STR: --- → yes
Keywords: regression, testcase
(Assignee)

Comment 9

a year ago
It appears the issue is that the first reflow in nsDocumentViewer::GetContentSizeInternal [1], which reflows the content with unlimited height (to get its full height) naturally sets the scroll top to zero, and the original scroll top is lost.

After the second reflow with maximum height specified, the scroll top stays at zero.

[1]: https://dxr.mozilla.org/mozilla-central/rev/5ca5691372cb432ec1fa4693ca608a30858226de/layout/base/nsDocumentViewer.cpp#3571
---

Thread 1 "firefox" hit Hardware watchpoint 13: *((int *) 0x7f4a122932ec)

Old value = -53280
New value = 0
mozilla::ScrollFrameHelper::ScrollToImpl (this=this@entry=0x7f4a12293708, aPt=..., aRange=..., aOrigin=0x7f4a1b3796e0, aOrigin@entry=0x0)
    at layout/generic/nsGfxScrollFrame.cpp:2821
2821	    nsLayoutUtils::CanScrollOriginClobberApz(mLastScrollOrigin) &&
(gdb) bt
#0  0x00007f4a221cf8ad in mozilla::ScrollFrameHelper::ScrollToImpl(nsPoint, nsRect const&, nsIAtom*) (this=this@entry=0x7f4a12293708, aPt=..., aRange=..., aOrigin=0x7f4a1b3796e0, 
    aOrigin@entry=0x0) at layout/generic/nsGfxScrollFrame.cpp:2821
#1  0x00007f4a221d2921 in mozilla::ScrollFrameHelper::ReflowFinished() (this=0x7f4a12293708) at layout/generic/nsGfxScrollFrame.cpp:5335
#2  0x00007f4a220b65b7 in mozilla::PresShell::HandlePostedReflowCallbacks(bool) (this=this@entry=0x7f4a12290000, aInterruptible=aInterruptible@entry=true)
    at layout/base/PresShell.cpp:4023
#3  0x00007f4a220b667b in mozilla::PresShell::DidDoReflow(bool) (this=this@entry=0x7f4a12290000, aInterruptible=aInterruptible@entry=true)
    at layout/base/PresShell.cpp:9183
#4  0x00007f4a220b6a20 in mozilla::PresShell::ResizeReflowIgnoreOverride(int, int, int, int) (this=0x7f4a12290000, aWidth=9060, aHeight=1073741823, aOldWidth=<optimized out>, aOldHeight=<optimized out>) at layout/base/PresShell.cpp:1987
#5  0x00007f4a220d0763 in nsDocumentViewer::GetContentSizeInternal(int*, int*, int, int) (this=this@entry=0x7f49f3c5f8b0, aWidth=aWidth@entry=0x7fffffff8bb8, aHeight=aHeight@entry=0x7fffffff8bd0, aMaxWidth=<optimized out>, aMaxHeight=36000) at layout/base/nsDocumentViewer.cpp:3571
#6  0x00007f4a220d0a5c in nsDocumentViewer::GetContentSizeConstrained(int, int, int*, int*) (this=0x7f49f3c5f8b0, aMaxWidth=800, aMaxHeight=600, aWidth=0x7fffffff8bb8, aHeight=0x7fffffff8bd0)
    at layout/base/nsDocumentViewer.cpp:3633
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8899278 - Flags: review?(tnikkel)
Attachment #8899278 - Flags: review?(dholbert)
Attachment #8899278 - Flags: review?(dbaron)
(Assignee)

Updated

a year ago
Attachment #8899278 - Flags: review?(xidorn+moz)
Attachment #8899278 - Flags: review?(bugs)

Comment 11

a year ago
mozreview-review
Comment on attachment 8899278 [details]
Bug 1324499 - Constrain height before calling DidDoReflow callbacks

https://reviewboard.mozilla.org/r/170520/#review175656

You don't need to ask so many people to review. One reviewer should be enough.

The change in this patch isn't in my familiar area, and it seems to be touching some core layout code. Although I can try to review, I would probably prefer asking dbaron or dholbert to have a look.

::: layout/base/PresShell.cpp:1899
(Diff revision 1)
>  {
>    static_cast<PresShell*>(aPresShell)->FireResizeEvent();
>  }
>  
>  nsresult
> -PresShell::ResizeReflow(nscoord aWidth, nscoord aHeight, nscoord aOldWidth, nscoord aOldHeight)
> +PresShell::ResizeReflow(nscoord aWidth, nscoord aHeight, nscoord aOldWidth, nscoord aOldHeight, bool aHeightIsConstraint)

This line seems to be too long. Please break it into multiple lines.

::: layout/base/PresShell.cpp:1915
(Diff revision 1)
>      // ResizeReflowIgnoreOverride if it changed.
>      mMobileViewportManager->RequestReflow();
>      return NS_OK;
>    }
>  
> -  return ResizeReflowIgnoreOverride(aWidth, aHeight, aOldWidth, aOldHeight);
> +  return ResizeReflowIgnoreOverride(aWidth, aHeight, aOldWidth, aOldHeight, aHeightIsConstraint);

So does this line.

::: layout/base/PresShell.cpp:1919
(Diff revision 1)
>  
> -  return ResizeReflowIgnoreOverride(aWidth, aHeight, aOldWidth, aOldHeight);
> +  return ResizeReflowIgnoreOverride(aWidth, aHeight, aOldWidth, aOldHeight, aHeightIsConstraint);
>  }
>  
>  nsresult
> -PresShell::ResizeReflowIgnoreOverride(nscoord aWidth, nscoord aHeight, nscoord aOldWidth, nscoord aOldHeight)
> +PresShell::ResizeReflowIgnoreOverride(nscoord aWidth, nscoord aHeight, nscoord aOldWidth, nscoord aOldHeight, bool aHeightIsConstraint)

and this.

::: layout/base/nsIPresShell.h:369
(Diff revision 1)
>  
>    /**
>     * Reflow the frame model into a new width and height.  The
>     * coordinates for aWidth and aHeight must be in standard nscoord's.
>     */
> -  virtual nsresult ResizeReflow(nscoord aWidth, nscoord aHeight, nscoord aOldWidth = 0, nscoord aOldHeight = 0) = 0;
> +  virtual nsresult ResizeReflow(nscoord aWidth, nscoord aHeight, nscoord aOldWidth = 0, nscoord aOldHeight = 0, bool aHeightIsConstraint = false) = 0;

and this.

::: layout/base/nsIPresShell.h:374
(Diff revision 1)
> -  virtual nsresult ResizeReflow(nscoord aWidth, nscoord aHeight, nscoord aOldWidth = 0, nscoord aOldHeight = 0) = 0;
> +  virtual nsresult ResizeReflow(nscoord aWidth, nscoord aHeight, nscoord aOldWidth = 0, nscoord aOldHeight = 0, bool aHeightIsConstraint = false) = 0;
>    /**
>     * Do the same thing as ResizeReflow but even if ResizeReflowOverride was
>     * called previously.
>     */
> -  virtual nsresult ResizeReflowIgnoreOverride(nscoord aWidth, nscoord aHeight, nscoord aOldWidth, nscoord aOldHeight) = 0;
> +  virtual nsresult ResizeReflowIgnoreOverride(nscoord aWidth, nscoord aHeight, nscoord aOldWidth, nscoord aOldHeight, bool aHeightIsConstraint = false) = 0;

and this.
Attachment #8899278 - Flags: review?(xidorn+moz)
Attachment #8899278 - Flags: review?(tnikkel)
Attachment #8899278 - Flags: review?(bugs)
Could you include an explanation of what the patch is trying to accomplish?
(Assignee)

Comment 13

a year ago
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #12)
> Could you include an explanation of what the patch is trying to accomplish?

You mean elaborate the commit message?
Yes.  Now that I had a little more time to look at the bug, I see that the first 3 lines of comment 9 contain a brief description of the underlying problem, but they don't explain what approach you took to fix it.
A few other comments though:

 - at a low level, I think the aHeightIsContstraint should, throughout the patch, be aHeightIs*Un*constrain*ed* (two changes).  But I might be misunderstanding

 - at a slightly higher level, it's not clear to me why you're not trying to fix the bug at a slightly higher level (in nsDocumentViewer::GetContentSizeInternal), by making the code that decides to reflow at an unconstrained height save and restore the scroll position.  (You may or may not want to use the nsIStatefulFrame implementation on the scroll frame to do that.)
Comment hidden (mozreview-request)
Comment on attachment 8899278 [details]
Bug 1324499 - Constrain height before calling DidDoReflow callbacks

https://reviewboard.mozilla.org/r/170520/#review175724

Please re-request review after addressing comment 12 / comment 14 / comment 15.
Attachment #8899278 - Flags: review?(dbaron)
(Assignee)

Comment 18

a year ago
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #15)
>  - at a low level, I think the aHeightIsContstraint should, throughout the
> patch, be aHeightIs*Un*constrain*ed* (two changes).  But I might be
> misunderstanding

I renamed the parameter to aHeightIsLimit: when true, aHeight is a limit to height
(i.e. the final height can be smaller) instead of a fixed dimension.

>  - at a slightly higher level, it's not clear to me why you're not trying to
> fix the bug at a slightly higher level (in
> nsDocumentViewer::GetContentSizeInternal), by making the code that decides
> to reflow at an unconstrained height save and restore the scroll position. 
> (You may or may not want to use the nsIStatefulFrame implementation on the
> scroll frame to do that.)

I thought it'd be better (?) if we simply don't lose the scroll position.

But I'll push another change with that fix; you can accept the one you prefer.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8899278 - Flags: review?(dholbert)
Attachment #8899278 - Flags: review?(dbaron)
(Assignee)

Comment 21

a year ago
Created attachment 8899462 [details]
scrollbug_with_events.xpi
(Assignee)

Comment 22

a year ago
I realised that the current way of reflowing to determine size causes a lot of spurious JS scroll & resize events.

Try out scrollbug_with_events.xpi and look in the addon's console.

I guess another bug should be filed about this? The fix might be to only fire resize/scroll events if the size/scroll position actually changed, which the code currently doesn't check for.
(Assignee)

Comment 23

a year ago
(In reply to grante.francois from comment #7)
> It's a big issue on our side too. The only thing I can do is to reduce the
> height of the popup the maximum I can but it's most of the time not really
> possible. It's really painful for the users.

FWIW as a workaround, you can wrap your whole popup contents in a div limited
to max-height 600px. The bug only affects the scroll of the root HTML frame,
so if your root HTML frame doesn't scroll, then the bug is avoided.
Comment on attachment 8899459 [details]
Bug 1324499: Save and restore scroll position after reflow in GetContentSizeInternal

https://reviewboard.mozilla.org/r/170734/#review176042

r=dbaron with the following change

::: layout/base/nsDocumentViewer.cpp:3595
(Diff revision 1)
>      NS_ENSURE_SUCCESS(rv, rv);
>  
>      shellArea = presContext->GetVisibleArea();
> +
> +    // the first reflow reset our scroll, now set it back
> +    if (frameState) {

For safety, please also add a test here that
presShell->GetRootScrollFrameAsScrollable() == scrollFrame (so we don't enter this codepath if the frame pointer is different).
Attachment #8899459 - Flags: review?(dbaron) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8899278 [details]
Bug 1324499 - Constrain height before calling DidDoReflow callbacks

https://reviewboard.mozilla.org/r/170520/#review176094
Attachment #8899278 - Flags: review?(dbaron)
(Assignee)

Updated

a year ago
Attachment #8899278 - Attachment is obsolete: true
(Assignee)

Comment 29

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca71f04513ab

Can someone add checkin-needed?
Assignee: nobody → angelsl

Comment 30

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/76c7aa772dc4
Save and restore scroll position after reflow in GetContentSizeInternal r=dbaron
https://hg.mozilla.org/mozilla-central/rev/76c7aa772dc4
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Assignee)

Comment 32

a year ago
Comment on attachment 8899459 [details]
Bug 1324499: Save and restore scroll position after reflow in GetContentSizeInternal

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1294442
[User impact if declined]: Webexts with popups that are larger than 600px content height may become unusable (e.g. uMatrix)
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: Install the webext in attachment 8819956 [details], open the popup, scroll down and click on a list item
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low
[Why is the change risky/not risky?]: Code changes a method currently only used by webext popups
[String changes made/needed]: None
Attachment #8899459 - Flags: approval-mozilla-beta?
status-firefox56: --- → affected
Flags: needinfo?(sescalante)
Flags: needinfo?(kmaglione+bmo)
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on the latest Nightly build? Thanks!
Flags: qe-verify+

Comment 34

a year ago
I can still reproduce this bug in Nightly 23-08-17. STR:
Install uMatrix beta https://addons.mozilla.org/en-US/firefox/addon/umatrix/versions/beta
Go to http://www.dailymail.co.uk/ushome/index.html
Open uMatrix dashboard, try to scroll down

AR: hovering over the matrix cells resets the scroll position to the top
This patch should be in the next Nightly.

Comment 36

a year ago
Nightly 24-08-17: I can no longer reproduce the bug using STR from comment 34. Great job, Angelsl! Thank you very much! \o/
Thanks for the patch, angelsl! Welcome onboard. :) Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition#August_2017

I look forward to seeing you around!
status-firefox55: --- → wontfix
status-firefox-esr52: --- → wontfix
Comment on attachment 8899459 [details]
Bug 1324499: Save and restore scroll position after reflow in GetContentSizeInternal

Fix a WebExtension related issue and was verified. Beta56+.
Attachment #8899459 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 39

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/cb753cf67c6c
status-firefox56: affected → fixed
I've reproduced this issue following the STR from comment 0, using an old Nightly build.

This is verified fixed on latest Nightly 57.0a1 (2017-09-13) and 56 Beta 11 (20170911193316) across platforms: Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 16.04 x64 LTS.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
status-firefox57: fixed → verified
Flags: qe-verify+

Updated

2 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.