292998 - fastback/bfcache should be invalidated after a text resize

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[Jo Hermans]

- open http://slashdot.org/
- resize the text size
- go to one of the articles
- notice that the new page is also using the new text size (ok)
- go back
- notice that the first page still has the new size (ok)
- resize the text back to normal
- go forward again
- notice that the second page still has the changed text size, not the normal

The text size has to work for all pages in the window or tab. This means that if
you change it, it should invalidate the cached pages in bfcache or it should at
least trigger a new text resize when you go back to that page.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

IMHO We should not disable the bfcache or evict pages already in there when the
text is resized. It is both faster and more consistent to just update the size
once we return to the cached page.

Comment 2

[Asa Dotzler [:asa]]

transferring nomination to 1.8b4. If we don't get it there, we don't get it for 1.1

Comment 3

[Brian Ryner (not reading)]

Targetting to fix for beta, this one should be relatively easy.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

maybe RestoreFromHistory can use nsDocShell::SetupNewViewer?

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

- I didn't use SetupNewViewer because that did a lot of stuff, some of which
should probably not be done here
- copying the other properties that are copied in SetupNewViewer is probably
not necessary for history traversal

- I'd have done the ClearStyleDataAndReflow call in docshell but the symbol is
not exported from layout.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Instead of the nsDocumentViewer.cpp change, why not move the storage of text
zoom from device context to pres context?  There doesn't seem to be a good
reason for it to be on the device context anymore.  (Given where it was stored,
why did we need to do all that work to persist between document viewers in the
past?  I guess it was for subframes.)

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: move textZoom to prescontext

I'm unsure whether the printing changes are correct... I don't know what their
point was (is the DC reused for printing and screen display?)

Comment 8

[Christian Persch (GNOME) (away; not receiving bug mail)]

Maybe I misunderstand this bug, but AIUI, it says that all pages in the fastback
cache should have the same zoom level set, i.e. you change zoom in one page and
go back/forward it should be applied to the other page as well. If this is
correct, this will break fastback cache for some embedders (Epiphany and Galeon
at least) which do apply different zoom levels on a *per-page basis*.
So, say, you go to page A with zoom z_A, load page B and change zoom to zoom
z_B, go back, change z_A, and forward again. Comment 0 says that the new z_A
should apply to page B; but the app wants to keep applying z_B. Will this patch
cause an unnecessary reflow because it tries to apply z_A to page B, and then
another one when the embedding app resets the zoom to z_B ?

(In reply to comment #7)
> Created an attachment (id=191807) [edit]

+  if (mPresContext && aTextZoom != mPresContext->TextZoom()) {
+      mPresContext->SetTextZoom(aTextZoom);

Just one thing I noticed in the patch: I thought one should never directly
compare floats with == or != ?

Comment 9

[Jo Hermans]

(In reply to comment #8)
> Maybe I misunderstand this bug, but AIUI, it says that all pages in the fastback
> cache should have the same zoom level set, i.e. you change zoom in one page and
> go back/forward it should be applied to the other page as well. If this is
> correct, this will break fastback cache for some embedders (Epiphany and Galeon
> at least) which do apply different zoom levels on a *per-page basis*.

I filed this because it would be the expected behaviour would be that when you
change the text-size (which works per window or tab), you would expect that it
works for the entire window/tab, pages in the cache included. Actually, the fact
that it didn't work for the cache until I reloaded, was proof for me that the
bfcache worked :-)

Those extensions (which implement this per page or per website) actually have
the same problem. You might change the text-size for a specific website, and
then go back in the cache. The cached copy should also have the changed text-size.

> So, say, you go to page A with zoom z_A, load page B and change zoom to zoom
> z_B, go back, change z_A, and forward again. Comment 0 says that the new z_A
> should apply to page B; but the app wants to keep applying z_B. Will this patch
> cause an unnecessary reflow because it tries to apply z_A to page B, and then
> another one when the embedding app resets the zoom to z_B ?
> 

No. The point is that when you go back to A, z_B should be applied. Currently,
the cache contains a copy of A with z_A.

When you have an extension that tries to force page A to use z_A, then it should
do so (invalidating the page when you go back, for example). But if you don't
have that extension, then it should always show it to me in the current zoom-level.

Comment 10

[Christian Persch (GNOME) (away; not receiving bug mail)]

(In reply to comment #9)
> I filed this because it would be the expected behaviour would be that when you
> change the text-size (which works per window or tab), you would expect that it
> works for the entire window/tab, pages in the cache included. Actually, the fact
> that it didn't work for the cache until I reloaded, was proof for me that the
> bfcache worked :-)
> 
> Those extensions (which implement this per page or per website) actually have
> the same problem. You might change the text-size for a specific website, and
> then go back in the cache. The cached copy should also have the changed text-size.

It's not an extension, it's an embedding application. What I'm saying is that
docshell shouldn't hardcode firefox client policy (to always apply the same zoon
to all pages in the tab).

> > So, say, you go to page A with zoom z_A, load page B and change zoom to zoom
> > z_B, go back, change z_A, and forward again. Comment 0 says that the new z_A
> > should apply to page B; but the app wants to keep applying z_B. Will this patch
> > cause an unnecessary reflow because it tries to apply z_A to page B, and then
> > another one when the embedding app resets the zoom to z_B ?
> > 
> 
> No. The point is that when you go back to A, z_B should be applied. Currently,
> the cache contains a copy of A with z_A.
Which is exactly how I want it :)

> When you have an extension that tries to force page A to use z_A, then it should
> do so (invalidating the page when you go back, for example).
Which will cause unnecessary reflows, slowing down back/forward for my application.

> But if you don't
> have that extension, then it should always show it to me in the current
zoom-level.
Yes, this is firefox zoom policy. So IMHO this should be done in frontend, not
enforced in docshell.

----

Anyway, I talked with biesi about this, and concluded that implementing this in
docshell for now is ok, but that is should be moved to frontend later.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: restore text zoom earlier, so that various notification handlers can override it

this sets the text zoom earlier, so that onpageshow etc handlers can override
it

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

HEAD:
Checking in docshell/base/nsDocShell.cpp;
/cvsroot/mozilla/docshell/base/nsDocShell.cpp,v  <--  nsDocShell.cpp
new revision: 1.722; previous revision: 1.721
done
Checking in gfx/public/nsDeviceContext.h;
/cvsroot/mozilla/gfx/public/nsDeviceContext.h,v  <--  nsDeviceContext.h
new revision: 1.40; previous revision: 1.39
done
Checking in gfx/public/nsIDeviceContext.h;
/cvsroot/mozilla/gfx/public/nsIDeviceContext.h,v  <--  nsIDeviceContext.h
new revision: 1.50; previous revision: 1.49
done
Checking in gfx/src/nsDeviceContext.cpp;
/cvsroot/mozilla/gfx/src/nsDeviceContext.cpp,v  <--  nsDeviceContext.cpp
new revision: 3.115; previous revision: 3.114
done
Checking in gfx/src/cairo/nsCairoDeviceContext.cpp;
/cvsroot/mozilla/gfx/src/cairo/nsCairoDeviceContext.cpp,v  <-- 
nsCairoDeviceContext.cpp
new revision: 1.12; previous revision: 1.11
done
Checking in layout/base/nsDocumentViewer.cpp;
/cvsroot/mozilla/layout/base/nsDocumentViewer.cpp,v  <--  nsDocumentViewer.cpp
new revision: 1.445; previous revision: 1.444
done
Checking in layout/base/nsPresContext.cpp;
/cvsroot/mozilla/layout/base/nsPresContext.cpp,v  <--  nsPresContext.cpp
new revision: 3.289; previous revision: 3.288
done
Checking in layout/base/nsPresContext.h;
/cvsroot/mozilla/layout/base/nsPresContext.h,v  <--  nsPresContext.h
new revision: 3.151; previous revision: 3.150
done
Checking in layout/printing/nsPrintData.cpp;
/cvsroot/mozilla/layout/printing/nsPrintData.cpp,v  <--  nsPrintData.cpp
new revision: 1.6; previous revision: 1.5
done
Checking in layout/printing/nsPrintData.h;
/cvsroot/mozilla/layout/printing/nsPrintData.h,v  <--  nsPrintData.h
new revision: 1.7; previous revision: 1.6
done
Checking in layout/printing/nsPrintEngine.cpp;
/cvsroot/mozilla/layout/printing/nsPrintEngine.cpp,v  <--  nsPrintEngine.cpp
new revision: 1.98; previous revision: 1.97
done
Checking in layout/style/nsStyleStruct.cpp;
/cvsroot/mozilla/layout/style/nsStyleStruct.cpp,v  <--  nsStyleStruct.cpp
new revision: 3.130; previous revision: 3.129
done

MOZILLA_1_8_BRANCH:
Checking in docshell/base/nsDocShell.cpp;
/cvsroot/mozilla/docshell/base/nsDocShell.cpp,v  <--  nsDocShell.cpp
new revision: 1.719.2.1; previous revision: 1.719
done
Checking in gfx/public/nsDeviceContext.h;
/cvsroot/mozilla/gfx/public/nsDeviceContext.h,v  <--  nsDeviceContext.h
new revision: 1.39.18.1; previous revision: 1.39
done
Checking in gfx/public/nsIDeviceContext.h;
/cvsroot/mozilla/gfx/public/nsIDeviceContext.h,v  <--  nsIDeviceContext.h
new revision: 1.49.20.1; previous revision: 1.49
done
Checking in gfx/src/nsDeviceContext.cpp;
/cvsroot/mozilla/gfx/src/nsDeviceContext.cpp,v  <--  nsDeviceContext.cpp
new revision: 3.114.12.1; previous revision: 3.114
done
Checking in gfx/src/cairo/nsCairoDeviceContext.cpp;
/cvsroot/mozilla/gfx/src/cairo/nsCairoDeviceContext.cpp,v  <-- 
nsCairoDeviceContext.cpp
new revision: 1.11.8.1; previous revision: 1.11
done
Checking in layout/base/nsDocumentViewer.cpp;
/cvsroot/mozilla/layout/base/nsDocumentViewer.cpp,v  <--  nsDocumentViewer.cpp
new revision: 1.442.4.1; previous revision: 1.442
done
Checking in layout/base/nsPresContext.cpp;
/cvsroot/mozilla/layout/base/nsPresContext.cpp,v  <--  nsPresContext.cpp
new revision: 3.288.12.1; previous revision: 3.288
done
Checking in layout/base/nsPresContext.h;
/cvsroot/mozilla/layout/base/nsPresContext.h,v  <--  nsPresContext.h
new revision: 3.150.4.1; previous revision: 3.150
done
Checking in layout/printing/nsPrintData.cpp;
/cvsroot/mozilla/layout/printing/nsPrintData.cpp,v  <--  nsPrintData.cpp
new revision: 1.5.14.1; previous revision: 1.5
done
Checking in layout/printing/nsPrintData.h;
/cvsroot/mozilla/layout/printing/nsPrintData.h,v  <--  nsPrintData.h
new revision: 1.6.28.1; previous revision: 1.6
done
Checking in layout/printing/nsPrintEngine.cpp;
/cvsroot/mozilla/layout/printing/nsPrintEngine.cpp,v  <--  nsPrintEngine.cpp
new revision: 1.97.4.1; previous revision: 1.97
done
Checking in layout/style/nsStyleStruct.cpp;
/cvsroot/mozilla/layout/style/nsStyleStruct.cpp,v  <--  nsStyleStruct.cpp
new revision: 3.129.6.1; previous revision: 3.129
done

Comment 13

[Jo Hermans]

This seems to be fixed in the latest branch release (1.8 branch, but the name
isn't changed yet). I tested by changing max_viewers and clearing the cache, and
the behaviour stayed the same.

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050813
Firefox/1.0+