727765 - [Page Thumbnails] thumbnails sometimes have a black border at the right

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

We need to take any kind of scrollbar into account when capturing screenshots.

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

This patch should detect when the vertical and horizontal scrollbars appear and then remove the scrollbar width if needed.

Comment 2

[Dão Gottwald [:dao]]

When exactly and why do scrollbars cause a black border?

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v2

(In reply to Dão Gottwald [:dao] from comment #2)
> When exactly and why do scrollbars cause a black border?

In order to draw the window content to a canvas, we would need to width and height of the content.  We are using the innerWidth and innerHeight from the contentWindow but that would include the scrollbars if they appear, and the ctx.drawWindow couldn't the scrollbars because they are not part of the dom content.

I have updated the code to use document.body.clientHeight/Width which should exclude the scrollbars.  However, not all content has document.body so innerHeight/Width would be used in that case.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

I know that we've had the same problems with Panorama and Asa once showed me a newtab page with multiple thumbnails having a block border at the right. Alas, I'm unable to come up with a manual test right now...

Raymond, can you please write a test for this? So that we can be sure this problem actually exists and we fixed it. We will probably need some combination of a specific page size and browser window size.

browser/components/thumbnails/test/head.js contains all the functions you need to capture a thumbnail for a given page and how to check the pixel colors.

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: Manual test screenshot

For manual tests, I have created two html pages: one with scrollbars (style="overflow: scroll;") and one without.  You can see the screenshot that there is a light grey bar appears on the right on the one with scrollbar.

If you don't see that, you would need to enlarge the window and it would appear.

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: Test page with scrollbars

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: Test page without scrollbars

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: v3

Added tests

Comment 9

[Raymond Lee [:raymondlee]]

Comment on attachment 622313 [details] [diff] [review]
v3

After applying this patch, browser/components/tabview/test/browser_tabview_bug594958.js fails and investigating now

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: v3

Comment 11

[Raymond Lee [:raymondlee]]

Comment on attachment 622329 [details] [diff] [review]
v3

I have spent some time trying to figure out why this patch affects browser/components/tabview/test/browser_tabview_bug594958.js, however, I couldn't find a connection between capturing code in PageThumbs and drawing canvas code in TabView.


However, if I delay the checking of checkUrl(test) in browser_tabview_bug594958, the tests pass.  Therefore, it might be a timing issue.

diff --git a/browser/components/tabview/test/browser_tabview_bug594958.js b/browser/components/tabview/test/browser_tabview_bug594958.js
--- a/browser/components/tabview/test/browser_tabview_bug594958.js
+++ b/browser/components/tabview/test/browser_tabview_bug594958.js
@@ -56,17 +56,17 @@ function test() {
 
     let browser = tab.linkedBrowser;
     browser.addEventListener("load", function onLoad(event) {
        browser.removeEventListener("load", onLoad, true);
        
        let tabItem = tab._tabViewTabItem;
        tabItem.addSubscriber("updated", function onUpdated() {
          tabItem.removeSubscriber("updated", onUpdated);
-         checkUrl(test);
+         setTimeout(function() { checkUrl(test); }, 1000);
        });
     }, true);
     browser.loadURI(test.url);
   }

@Tim: do you see any reasons what cause the problem?

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 622329 [details] [diff] [review]
v3

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

::: browser/components/thumbnails/PageThumbs.jsm
@@ +177,5 @@
>     * @param aWindow The content window.
>     * @return An array containing width, height and scale.
>     */
>    _determineCropSize: function PageThumbs_determineCropSize(aWindow) {
> +    let body = aWindow.document.body;

This should better be |let body = aWindow.document && aWindow.document.body|

::: browser/components/thumbnails/test/Makefile.in
@@ +20,4 @@
>  	head.js \
>  	background_red.html \
>  	background_red_redirect.sjs \
> +	with_scrollbars.html \

capture_with_scrollbars.html might be a better name for that.

::: browser/components/thumbnails/test/browser_thumbnails_bug727765.js
@@ +1,1 @@
> +const URL = "http://mochi.test:8888/browser/browser/components/thumbnails/" +

We're missing the public license header here.

@@ +1,5 @@
> +const URL = "http://mochi.test:8888/browser/browser/components/thumbnails/" +
> +            "test/with_scrollbars.html";
> +
> +/**
> + * These tests ensure that thumbnails don't have right border created by scrollbar.

Nit: "right borders"

@@ +9,5 @@
> +  yield addTab(URL);
> +  // Wait until the thumbnail's file has been written.
> +  yield whenFileExists(URL);
> +  // Check the thumbnail color of the bottom right pixel.
> +  yield checkThumbnailColor(URL, 255, 0, 0, "It has a red thumbnail with no border on the right");

Better: "we have a red thumbnail..."

@@ +12,5 @@
> +  // Check the thumbnail color of the bottom right pixel.
> +  yield checkThumbnailColor(URL, 255, 0, 0, "It has a red thumbnail with no border on the right");
> +}
> +
> +function whenFileExists(aURL) {

This is now the third test that needs this function (also _storage.js and _redirect.js). We should move that to head.js and include the |file.fileSize| check from _storage.js.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Raymond Lee [:raymondlee] from comment #11)
> @Tim: do you see any reasons what cause the problem?

So, interestingly, if you insert:

> let foo = win.document.body.clientWidth;

before this line here:

http://mxr.mozilla.org/mozilla-central/source/browser/components/tabview/tabitems.js#1458

then window.innerWidth will return a different value. This is probably because accessing clientWidth flushes pending layout changes - which is a resize of the window in this case.

Comment 14

[Raymond Lee [:raymondlee]]

Attachment: v4

Updated the patch based on #comment 12

Comment 15

[Raymond Lee [:raymondlee]]

(In reply to Tim Taubert [:ttaubert] from comment #13)
> (In reply to Raymond Lee [:raymondlee] from comment #11)
> > @Tim: do you see any reasons what cause the problem?
> 
> So, interestingly, if you insert:
> 
> > let foo = win.document.body.clientWidth;
> 
> before this line here:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/components/tabview/
> tabitems.js#1458
> 
> then window.innerWidth will return a different value. This is probably
> because accessing clientWidth flushes pending layout changes - which is a
> resize of the window in this case.

Although the clientWidth would trigger layout changes, it still doesn't fix the issues browser_tabview_bug594958.js.  I am still not able to figure out the root cause :S

Comment 16

[Thomas Stache]

Attachment: Black bars encoded into thumbnail PNG files

Unfortunately this bug stalled 5 months ago - what is missing to bring it to review?
(Here's a screenshot from the current Aurora Windows build)

Comment 17

[Asa Dotzler [:asa]]

Tim, Raymond, is the screenshot in Thomas' attachment the same issue as this bug? If so, I was just going to report this. It's a rather glaring bug. Are either of you able to dig further into it?

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Asa Dotzler [:asa] from comment #17)
> Tim, Raymond, is the screenshot in Thomas' attachment the same issue as this
> bug? If so, I was just going to report this. It's a rather glaring bug. Are
> either of you able to dig further into it?

Yes, this looks exactly like this bug. I won't have time to investigate this... Raymond?

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 1 - use nsIDOMWindowUtils.getScrollbarWidth() to avoid black borders at the right

Stealing.

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 713361 [details] [diff] [review]
part 1 - use nsIDOMWindowUtils.getScrollbarWidth() to avoid black borders at the right

Tiny fix. The session store code added to head.js is because of the asynchronous read of sessionstore.js that somehow makes single tests fail (sometimes).

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 2 - let Panorama use PageThumbs to draw screenshots to TabCanvases

browser_tabview_bug594958.js failed horribly on try (I have no idea why) and I didn't want to spend much time on fixing that. I remember that the test is quite old and very hacky/flaky. If we let Panorama use PageThumbs, we can remove the test as it doesn't apply anymore and we can remove a whole bunch of custom canvas/screenshot code as well.

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 713861 [details] [diff] [review]
part 2 - let Panorama use PageThumbs to draw screenshots to TabCanvases

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

::: browser/components/thumbnails/PageThumbs.jsm
@@ +144,5 @@
>      let [sw, sh, scale] = this._determineCropSize(aWindow, aCanvas);
>      let ctx = aCanvas.getContext("2d");
>  
>      // Scale the canvas accordingly.
> +    ctx.save();

Is this (and the ctx.restore) fixing a known bug?

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Jared Wein [:jaws] from comment #22)
> >      let [sw, sh, scale] = this._determineCropSize(aWindow, aCanvas);
> >      let ctx = aCanvas.getContext("2d");
> >  
> >      // Scale the canvas accordingly.
> > +    ctx.save();
> 
> Is this (and the ctx.restore) fixing a known bug?

It's fixing a bug I didn't know of until Panorama as the first client re-using a canvas came around :)

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

Currently trying to figure out some weird Windows orange that I can't reproduce of course :(

https://tbpl.mozilla.org/?tree=Try&rev=736e540fa67a

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Ok, I just pushed it to try subtracting the horizontal scrollbar's height and it works on Windows now, too. Looks like we need another API for that, yay! I'm going to file a bug.

https://tbpl.mozilla.org/?tree=Try&rev=e48638c31063

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 1 - use nsIDOMWindowUtils.getScrollbarSize() to avoid black borders at the right

Minor change to PageThumbs.jsm in part 1: getScrollbarWidth() doesn't exist anymore - we now use getScrollbarSize(). That means we also decrease the available space when having vertical scrollbars.

Comment 27

[Dão Gottwald [:dao]]

Comment on attachment 718336 [details] [diff] [review]
part 1 - use nsIDOMWindowUtils.getScrollbarSize() to avoid black borders at the right

>+    let sw = aWindow.innerWidth - sbWidth.value;

Doesn't this always subtract the scrollbar width on the right side even if the scrollbar is on the left, i.e. in RTL content?

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #27)
> Doesn't this always subtract the scrollbar width on the right side even if
> the scrollbar is on the left, i.e. in RTL content?

The available width does decrease for LTR and RTL but you're right that the black border would not be prevented on the left for RTL mode. Basically, PageThumbs._determineCropSize() just needs to return an offsetX as well.

> if (isRTL && sw < availableWidth) {
>   offsetX = sbWidth.value + (availableWidth - sw);
> }

That's an easy thing to do (thank you for catching this, btw) but the harder part is detecting the RTL mode. We want to avoid black borders and therefore don't care if the page is RTL but if the whole browser UI is RTL, right?

I guess we can do it like this?
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/browserPlacesViews.js#715

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #28)
> I guess we can do it like this?
> http://mxr.mozilla.org/mozilla-central/source/browser/components/places/
> content/browserPlacesViews.js#715

Or should we rather use nsIXULChromeRegistry?
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/layoutview/view.xhtml#37

Comment 30

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #28)
> don't care if the page is RTL but if the whole browser UI is RTL, right?

I'm not sure, I would expect that the content direction matters.

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #30)
> (In reply to Tim Taubert [:ttaubert] from comment #28)
> > don't care if the page is RTL but if the whole browser UI is RTL, right?
> 
> I'm not sure, I would expect that the content direction matters.

Yeah I just figured that, too. The offsetX needs to include the scrollbar width if the UI is in RTL mode. If the content page is in RTL mode we need to add (availableWidth - sw) to offsetX so that the thumbnail is right-aligned. So those are two separate steps.

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

So... even sites like aljazeera.net (and a couple others I tested) are really hard to determine as RTL. They use some tables with align=right, text-align:right, text-direction:rtl on some specific elements, etc. The <body>'s computed style is definitely not RTL.

I think it should be sufficient if we only focus on not having black borders on the left?

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 3 - adjust left offset when capturing thumbnails in RTL locales

Comment 34

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #32)
> So... even sites like aljazeera.net (and a couple others I tested) are
> really hard to determine as RTL. They use some tables with align=right,
> text-align:right, text-direction:rtl on some specific elements, etc. The
> <body>'s computed style is definitely not RTL.

aljazeera.net also has the scrollbar on the right, even with the UI direction being RTL, so this seems like a case we /shouldn't/ detect as RTL here.

Comment 35

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #34)
> aljazeera.net also has the scrollbar on the right, even with the UI
> direction being RTL, so this seems like a case we /shouldn't/ detect as RTL
> here.

But that's only because our browser UI isn't RTL, right? In Arabic locales it should be on the left. So there's that difference between the browser UI and the page we're viewing.

Comment 36

[Dão Gottwald [:dao]]

I set intl.uidirection.en to rtl, opened a new window, loaded aljazeera.net and the scrollbar was on the right.

Comment 37

[Tim Taubert [:ttaubert] (inactive)]

Ok, that is strange. Do we ever have scrollbars on the left for RTL pages? I have a left scrollbar for about:config (which is probably just a scrollbux) but not for any of the Arabic news sites (and RTL test cases) I tried. Is this something the operating system locale needs to support?

Comment 38

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #37)
> Ok, that is strange. Do we ever have scrollbars on the left for RTL pages? I
> have a left scrollbar for about:config (which is probably just a scrollbux)
> but not for any of the Arabic news sites (and RTL test cases) I tried. Is
> this something the operating system locale needs to support?

Maybe Ehsan knows.

Comment 39

[(no longer active)]

We don't currently put the scrollbar on the left for any pages.

Comment 40

[Dão Gottwald [:dao]]

Comment on attachment 718378 [details] [diff] [review]
part 3 - adjust left offset when capturing thumbnails in RTL locales

Can you add a comment saying that we put scrollbars on the right even for RTL content?

Comment 41

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 718336 [details] [diff] [review]
part 1 - use nsIDOMWindowUtils.getScrollbarSize() to avoid black borders at the right

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

::: toolkit/components/thumbnails/PageThumbs.jsm
@@ +240,5 @@
>    _determineCropSize: function PageThumbs_determineCropSize(aWindow, aCanvas) {
> +    let utils = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                       .getInterface(Ci.nsIDOMWindowUtils);
> +    let sbWidth = {}, sbHeight = {};
> +    utils.getScrollbarSize(true, sbWidth, sbHeight);

The previous version of this patch did not force a layout flush. Why the change?

Comment 42

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #40)
> Can you add a comment saying that we put scrollbars on the right even for
> RTL content?

Yes, will do.

Comment 43

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Jared Wein [:jaws] from comment #41)
> > +    let sbWidth = {}, sbHeight = {};
> > +    utils.getScrollbarSize(true, sbWidth, sbHeight);
> 
> The previous version of this patch did not force a layout flush. Why the
> change?

Thanks for spotting this. I think I changed this will trying to fix the intermittent Windows orange. I'll push it to try with flush=false to see if the tests are still green and if so I'll land that because I think that's what we prefer anyway.

Comment 44

[Tim Taubert [:ttaubert] (inactive)]

Looking good:

https://tbpl.mozilla.org/?tree=Try&rev=bb99ec56d541

Comment 45

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/6fa1760626ed
https://hg.mozilla.org/integration/fx-team/rev/1160886bdbf0

Comment 46

[Tim Taubert [:ttaubert] (inactive)]

Hmm. Someone merged but did not close this bug.

https://hg.mozilla.org/mozilla-central/rev/6fa1760626ed
https://hg.mozilla.org/mozilla-central/rev/1160886bdbf0