Closed Bug 611556 Opened 9 years ago Closed 6 years ago

zoom levels should persist within a session

Categories

(Firefox for Android :: Toolbar, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 31
Tracking Status
fennec + ---

People

(Reporter: dietrich, Assigned: esawin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 6 obsolete files)

When I zoom a page, then click on a link, then go back, the original page doesn't have the zoom level I left it at.

The zoom level should persist during a browsing session.
tracking-fennec: --- → ?
mike, could this be related to the fb cache stuff you were looking at?
Assignee: nobody → mkristoffersen
tracking-fennec: ? → 2.0-
Assignee: mozstuff → wjohnston
Whiteboard: [fennec-4.1?]
tracking-fennec: - → 7+
Whiteboard: [fennec-4.1?]
tracking-fennec: 7+ → +
Duplicate of this bug: 679661
Assignee: wjohnston → nobody
Component: Panning/Zooming → General
Product: Fennec → Fennec Native
QA Contact: pan-zoom → general
tracking-fennec: + → 16+
OS: Linux → Android
Hardware: x86 → ARM
Duplicate of this bug: 769855
tracking-fennec: 16+ → ?
Duplicate of this bug: 724770
tracking-fennec: ? → +
Duplicate of this bug: 843009
Assignee: nobody → esawin
QA Contact: esawin
Status: NEW → ASSIGNED
Attached patch (WIP) Frontend zoom sessions (obsolete) — Splinter Review
With this patch, we enable storing zoom session history in the frontend code, without touching backend interfaces and events.

The advantage I see with this approach is that it's easy to maintain (does not rely on full-stack event passing) and it does not affect desktop, where it's not really a use case.

A disadvantage is the minor overhead of |_zoomSessions| storing and handling.

Would this be a viable approach or should we look into a different solution, which uses a backend-propagated event to restore the zoom level?
Attachment #8402777 - Flags: feedback?(snorp)
Attachment #8402777 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8402777 [details] [diff] [review]
(WIP) Frontend zoom sessions

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

This seems alright, I guess. You'll of course need to handle the Purge and GotoIndex cases as well, otherwise stuff will get hosed up when people use the history API.
Attachment #8402777 - Flags: feedback?(snorp) → feedback+
(In reply to Eugen Sawin [:esawin] from comment #6)
> Created attachment 8402777 [details] [diff] [review]
> (WIP) Frontend zoom sessions
> 
> With this patch, we enable storing zoom session history in the frontend
> code, without touching backend interfaces and events.

Yeah but this means we'll have to reimplement it for every platform. I'd rather do it in the backend once and be done with it.

> The advantage I see with this approach is that it's easy to maintain (does
> not rely on full-stack event passing) and it does not affect desktop, where
> it's not really a use case.

It's not a use case right now, but will be in the future.

> A disadvantage is the minor overhead of |_zoomSessions| storing and handling.

Another disadvantage is that this approach is brittle because the history index could get out of sync with the zoom index.

> Would this be a viable approach or should we look into a different solution,
> which uses a backend-propagated event to restore the zoom level?

I would prefer to look into the backend approach unless you have a compelling argument against.
Attachment #8402777 - Flags: feedback?(bugmail.mozilla) → feedback-
Attached patch (WIP) Backend zoom sessions (obsolete) — Splinter Review
With this patch, we mimic the way scrolling position is stored to keep zoom levels persistent. For this, we add the zoom level to |nsPresState| and providing access to it via |nsGlobalWindow|.
Attachment #8403949 - Flags: feedback?(snorp)
Attachment #8403949 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8403949 [details] [diff] [review]
(WIP) Backend zoom sessions

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

Yeah, I like this better, but you should also get feedback from people who know the gecko pieces better. I think that the bits in browser.js are probably fine for now but eventually we'll want to change it so that the layout engine automatically paints at the restored zoom and notifies the front-end, just like it does with the scroll offset. That might require moving the "default initial zoom" code from the frontend (browser.js and TabChild.cpp) into nsGlobalWindow though so is probably best done as a follow-up.
Attachment #8403949 - Flags: feedback?(bzbarsky)
Attachment #8403949 - Flags: feedback?(bugmail.mozilla)
Attachment #8403949 - Flags: feedback+
Comment on attachment 8403949 [details] [diff] [review]
(WIP) Backend zoom sessions

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

It looks like the changes to nsGlobalWindow and nsIDOMWindow would add a 'zoom' attribute that is accessible by content, and I don't think we want that. We probably need to use something else to get that value into our browser.js code (nsIDOMWindowUtils or something).

::: mobile/android/chrome/content/browser.js
@@ +4211,5 @@
>  
>      sendMessageToJava(message);
>    },
>  
> +  restoredZoom: function() {

Maybe it's just me, but I don't like this "return float if success, null otherwise" kind of behavior. I'd rather rely on a separate shouldRestoreZoom() call to determine whether or not we want this value.
Attachment #8403949 - Flags: feedback?(snorp) → feedback-
Comment on attachment 8403949 [details] [diff] [review]
(WIP) Backend zoom sessions

> (nsIDOMWindowUtils or something)

We could just make it a [ChromeOnly] attribute in Window.webidl, if we're willing to depend on WebIDL window (which should land sometime in the next few weeks).

Please don't add anything scriptable to nsIDOMWindow.idl.
Attachment #8403949 - Flags: feedback?(bzbarsky) → feedback+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #11)
> It looks like the changes to nsGlobalWindow and nsIDOMWindow would add a
> 'zoom' attribute that is accessible by content, and I don't think we want
> that. We probably need to use something else to get that value into our
> browser.js code (nsIDOMWindowUtils or something).

Moving it to |nsIDOMWindowUtils|.

> ::: mobile/android/chrome/content/browser.js
> @@ +4211,5 @@
> >  
> >      sendMessageToJava(message);
> >    },
> >  
> > +  restoredZoom: function() {
> 
> Maybe it's just me, but I don't like this "return float if success, null
> otherwise" kind of behavior. I'd rather rely on a separate
> shouldRestoreZoom() call to determine whether or not we want this value.

I don't like it either and have been looking into a way to completely avoid having to check this without obscuring the zoom restoring, but couldn't find one.

Adding |shouldRestoreZoom| would mean that it becomes unsafe to use the restored zoom without calling it first, as there is no universal default value for the zoom which would not break things.
So we end up with a dependency between both calls, resulting in the code patterns:

if (this.shouldRestoreZoom()) {
  zoom = this.restoredZoom();
} else {
  zoom = somethingElse;
}

I think having |restoredZoom| return |null| is safer, as it will break things on spot, instead of overriding the zoom silently when used incorrectly, and it allows for more readable shortcut patterns like:

zoom = this.restoredZoom() || somethingElse;

But I'll look into alternative ways to restore the value for the next patch iteration, and of course, I'm open for suggestions!
Attached patch (WIP) Backend zoom sessions (obsolete) — Splinter Review
Moved the zoom property to |nsIDOMWinduwUtils|, taking feedback into account.
Attachment #8403949 - Attachment is obsolete: true
Attached patch Backend zoom sessions (obsolete) — Splinter Review
After some discussions, we have decided not to expose the compositor zoom level via |nsIDOMWindowUtils|, but instead to leverage the document resolution set in |nsPresShell|, which is directly derived from the zoom level.

Previously, the resolution has been only cached in |nsPresShell|. To make zoom levels persistent in history sessions, we synchronize the resolution via |nsIScrollableFrame| and fallback to the pres shell cache, when it's not available.
Attachment #8407703 - Flags: feedback?(snorp)
Attachment #8407703 - Flags: feedback?(roc)
CC'ing :tn and :mattwoodrow as well in case they have concerns about this. I would be very happy if we can move the resolution off the presshell and into scrollframes completely; this seems like a first step in that direction so I like it.
Comment on attachment 8407703 [details] [diff] [review]
Backend zoom sessions

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

I think this is lovely, but let's see what the layout folks say :)

::: dom/base/nsDOMWindowUtils.cpp
@@ +545,5 @@
> +
> +  nsIScrollableFrame* sf = presShell->GetRootScrollFrameAsScrollable();
> +  if (sf) {
> +    sf->SetResolution(gfxSize(aXResolution, aYResolution)); 
> +  } 

whitespace nit
Attachment #8407703 - Flags: review+
Comment on attachment 8407703 [details] [diff] [review]
Backend zoom sessions

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

I'm not sold on the idea of supporting zooming of arbitrary scrollable elements. However, I'm OK with the idea of moving the zoom state into the root scroll frame so it can be saved/restored via nsPresState.

::: dom/base/nsDOMWindowUtils.cpp
@@ +547,5 @@
> +  if (sf) {
> +    sf->SetResolution(gfxSize(aXResolution, aYResolution)); 
> +  } 
> +
> +  return presShell->SetResolution(aXResolution, aYResolution);

If there's no rootscrollframe we don't need to do anything. Everything has a rootscrollframe except for XUL documents and printed documents and neither of those need zooming support. So we shouldn't call nsIPresShell::SetResolution at all and we should remove it.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1599,5 @@
>    , mDestination(0, 0)
>    , mScrollPosAtLastPaint(0, 0)
>    , mRestorePos(-1, -1)
>    , mLastPos(-1, -1)
> +  , mResolution(0.0, 0.0)

Shouldn't this be 1.0, 1.0?

::: layout/generic/nsIScrollableFrame.h
@@ +135,5 @@
>     */
>    virtual nsSize GetScrollPositionClampingScrollPortSize() const = 0;
>  
>    /**
> +   * Get the document resolution.

You mean the element resolution, right?
Attachment #8407703 - Flags: feedback?(roc) → feedback+
Updated patch according to the feedback.
Attachment #8404932 - Attachment is obsolete: true
Attachment #8407703 - Attachment is obsolete: true
Attachment #8407703 - Flags: feedback?(snorp)
Attachment #8408389 - Flags: review?(roc)
Comment on attachment 8408389 [details] [diff] [review]
Add persistent zoom history sessions

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

::: dom/base/nsDOMWindowUtils.cpp
@@ +547,5 @@
> +
> +  if (sf) {
> +    sf->SetResolution(gfxSize(aXResolution, aYResolution)); 
> +    presShell->SetResolution(aXResolution, aYResolution);
> +  } 

Remove trailing whitespace

::: layout/generic/nsGfxScrollFrame.cpp
@@ +4533,5 @@
>  {
>    mRestorePos = aState->GetScrollState();
>    mDidHistoryRestore = true;
>    mLastPos = mScrolledFrame ? GetLogicalScrollPosition() : nsPoint(0,0);
> +  mResolution = aState->GetResolution();

Do we need to sync the PresShell resolution with our restored resolution here, to ensure these resolutions stay in sync? I think we probably do.

Alternatively, can we just get rid of the PresShell resolution entirely and always get it from the root scroll frame? I think we probably should.
Attachment #8408389 - Flags: review?(roc) → review-
Attached patch Backend zoom sessions (obsolete) — Splinter Review
I've removed the trailing whitespace from the edited lines and added |nsPresShell| resolution synchronization when restoring from |nsPresState|.

I think we agreed that removing the resolution from |nsPresShell| is a good move, but would prefer to split that into an additional refactoring patch. There are a few places that depend on |GetCumulativeResolution|, which need to be refactored, so I think it would be cleaner to have this new feature separated from that.
Attachment #8408389 - Attachment is obsolete: true
Attachment #8409169 - Flags: review?(roc)
Comment on attachment 8409169 [details] [diff] [review]
Backend zoom sessions

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +4534,5 @@
>    mRestorePos = aState->GetScrollState();
>    mDidHistoryRestore = true;
>    mLastPos = mScrolledFrame ? GetLogicalScrollPosition() : nsPoint(0,0);
> +  mResolution = aState->GetResolution();
> +  mOuter->PresContext()->PresShell()->SetResolution(mResolution.width, mResolution.height);

Sync with presshell only when mIsRoot!
Attachment #8409169 - Flags: review?(roc) → review-
Thanks for the review!

I'll file the follow-up patch to refactor the resolution out of the presshell when this one lands.
Attachment #8409169 - Attachment is obsolete: true
Attachment #8409916 - Flags: review?(roc)
I tried pinging in IRC but didn't get a response. I assume only the backend patch needs landing? Should this be leave-open for the frontend patch?
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24)
> I tried pinging in IRC but didn't get a response. I assume only the backend
> patch needs landing? Should this be leave-open for the frontend patch?

Sorry for the confusion! As discussed on IRC, the frontend patch is just an alternative implementation, so we should just land the backend patch.
Attachment #8402777 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/290e0e14312f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Component: General → Graphics, Panning and Zooming
Depends on: 1002426
You need to log in before you can comment on or make changes to this bug.