578616 - Taskbar tab previews have clipped tab contents

Status: RESOLVED WORKSFORME

(Firefox :: General, defect, P3)

Description

[Simek]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:2.0b1) Gecko/20100630 Firefox/4.0b1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:2.0b1) Gecko/20100630 Firefox/4.0b1

Tabs are below bookmarks. When FF 4.0b1 is opened and I move cursor to the taskbar FF icon the preview boxes appear - one for every tab. Now when I hover on one of them FF in background switches to this tab, but the site is 6px or 8px above the normal position and hover part of tabs.

Reproducible: Always

Steps to Reproduce:
1. Open FF
2. Set tab not "On the top"
3. Open at least 2 tab
4. Go to taskbar and hover tab preview
5. Look at FF in background



Screenshot: http://i27.tinypic.com/wwjjg3.png

Comment 1

[Simek]

Glitch also exist when tabs are on the top.

Comment 2

[[not reading bugmail]]

I can confirm I see this too on recent hourly.  The whole content area is shifting up on the Aero peek preview, scrollbars and all.

Comment 3

[Jim Mathies [:jimm]]

Might not be related toi the new titlebar, I don't see any relationship between the black margins and the non client margins. cc'ing roc, might be due to retained layers.

Comment 4

[Jim Mathies [:jimm]]

Odd, this only shows up on certain sites, bugzilla happens to be one of them.

Comment 5

[Jim Mathies [:jimm]]

Attachment: tab/live previews anomoly

Comment 6

[Jim Mathies [:jimm]]

Ahh, nope nope nope. I had bugzilla zoom set. :) I think this is the tab preview zoom problem.

Comment 7

[Rob Arnold [:robarnold]]

(In reply to comment #6)
> Ahh, nope nope nope. I had bugzilla zoom set. :) I think this is the tab
> preview zoom problem.

Looks like it to me.

Comment 8

[Daniel Jost]

Are you shure this is a duplicate? The original reportet problem (content shifted upwards) happens to me without any zoom set (4.0b1). In addition, Jim's screenshot seems to show a completely unrelated problem.

Comment 9

[Simek]

It's not an duplicate...

Comment 10

[Jim Mathies [:jimm]]

(In reply to comment #8)
> Are you shure this is a duplicate? The original reportet problem (content
> shifted upwards) happens to me without any zoom set (4.0b1). In addition, Jim's
> screenshot seems to show a completely unrelated problem.

I can't reproduce without zoom on a particular site set. Thumbnails display correctly. Can you screenshot a set of previews maybe for your case?

Comment 11

[Jim Mathies [:jimm]]

(In reply to comment #0)
> User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US;
> rv:2.0b1) Gecko/20100630 Firefox/4.0b1
> Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US;
> rv:2.0b1) Gecko/20100630 Firefox/4.0b1
> 
> Tabs are below bookmarks. When FF 4.0b1 is opened and I move cursor to the
> taskbar FF icon the preview boxes appear - one for every tab. Now when I hover
> on one of them FF in background switches to this tab, but the site is 6px or
> 8px above the normal position and hover part of tabs.
> 
> Reproducible: Always
> 
> Steps to Reproduce:
> 1. Open FF
> 2. Set tab not "On the top"
> 3. Open at least 2 tab
> 4. Go to taskbar and hover tab preview
> 5. Look at FF in background
> 
> 
> 
> Screenshot: http://i27.tinypic.com/wwjjg3.png

Doh, never mind. So could you try a nightly for this? This might be resolved fixed from another bug.

Comment 12

[Daniel Jost]

> Doh, never mind. So could you try a nightly for this? This might be resolved
> fixed from another bug.

Still present in Mozilla/5.0 (Windows; Windows NT 6.1; Win64; x64; rv:2.0b2pre) Gecko/20100720 Minefield/4.0b2pre

Comment 13

[Jim Mathies [:jimm]]

(In reply to comment #12)
> > Doh, never mind. So could you try a nightly for this? This might be resolved
> > fixed from another bug.
> 
> Still present in Mozilla/5.0 (Windows; Windows NT 6.1; Win64; x64; rv:2.0b2pre)
> Gecko/20100720 Minefield/4.0b2pre

and the key problem is that w/tabs on bottom, tabs get clipped by the page content? (Which is highlighted in your screenshot.)

Comment 14

[Jim Mathies [:jimm]]

Ahh, confirmed on a nightly.

- maximized window
- live previews have clipped tabs
- the browser is fine

Comment 15

[[not reading bugmail]]

(In reply to comment #14)
> Ahh, confirmed on a nightly.
> 
> - maximized window
> - live previews have clipped tabs
> - the browser is fine

Yup, also see comment 2 for additional info.

Comment 16

[Rob Arnold [:robarnold]]

Did some minimal debugging today. When unmaximized, we draw the tab contents 2px too high. When maximized, we draw them 8px too high. Jim, I think this is due to your nonclient margins change. It seems that the logical origin of the browser chrome is no longer the same as the image origin.

Comment 17

[Jim Mathies [:jimm]]

(In reply to comment #16)
> Did some minimal debugging today. When unmaximized, we draw the tab contents
> 2px too high. When maximized, we draw them 8px too high. Jim, I think this is
> due to your nonclient margins change. It seems that the logical origin of the
> browser chrome is no longer the same as the image origin.

The origin of the window is -8x-8 on the screen when maximized. With non-client margins, the content sits at the origin. Any idea where in the code we calculate the tab location for drawing?

Comment 18

[Jim Mathies [:jimm]]

(In reply to comment #17)
> (In reply to comment #16)
> > Did some minimal debugging today. When unmaximized, we draw the tab contents
> > 2px too high. When maximized, we draw them 8px too high. Jim, I think this is
> > due to your nonclient margins change. It seems that the logical origin of the
> > browser chrome is no longer the same as the image origin.
> 
> The origin of the window is -8x-8 on the screen when maximized. With non-client
> margins, the content sits at the origin. Any idea where in the code we
> calculate the tab location for drawing?

Looks like it might be here - 

    ctx.drawWindow(this.win.win, 0, 0, width, height, "transparent");

    // Compositor, where art thou?
    // Draw the tab content on top of the toplevel window
    this.updateCanvasPreview();

    let boxObject = this.linkedBrowser.boxObject;
    ctx.translate(boxObject.x, boxObject.y);
    ctx.drawImage(this.canvasPreview, 0, 0);

Might the box object values be off kilter from the 0,0 origin of the drawWindow call, since the y origin of the window is -8?

Comment 19

[Rob Arnold [:robarnold]]

(In reply to comment #18)
> Might the box object values be off kilter from the 0,0 origin of the drawWindow
> call, since the y origin of the window is -8?

When not maximized, the origin appears to be -2. I took screenshots of both states and the values of boxObject indicate that it expects the origin's y coordinate to be the row with the first orange pixel.

Comment 20

[Rob Arnold [:robarnold]]

(In reply to comment #19)
> When not maximized, the origin appears to be -2. I took screenshots of both
> states and the values of boxObject indicate that it expects the origin's y
> coordinate to be the row with the first orange pixel.

In simpler terms, the origin for boxObject does not take the margin into account. This is our problem - we now have a XUL <window> with non-zero margin.

Comment 21

[Rob Arnold [:robarnold]]

Attachment: Slight hack v1

So I can take the computed style for the margin properties and use that but this solution feels a bit hackish to me. I am wondering if there is a better way to determine the actual offset (i.e. some property I've overlooked).

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 460442 [details] [diff] [review]
Slight hack v1

>diff --git a/browser/components/wintaskbar/WindowsPreviewPerTab.jsm b/browser/components/wintaskbar/WindowsPreviewPerTab.jsm

>   previewTabCallback: function (ctx) {

>+    let elem = domWin.document.getElementById('main-window');

document.documentElement

>+    function extractPixelProperty(prop) {
>+      let prop = domWin.getComputedStyle(elem,null).getPropertyValue(prop);
>+      return parseInt(prop.slice(0,-2));

No need to slice, parseInt will ignore the trailing "px".

>+    let x = boxObject.x + extractPixelProperty('margin-left');
>+    let y = boxObject.y + extractPixelProperty('margin-top');
>+    ctx.translate(x, y);

The x case isn't actually needed, is it? We don't set horizontal margins on the window, and I'm not sure there would ever be a reason to. (If we did, would it be -moz-margin-start? Probably not, I guess...)

Comment 23

[Jim Mathies [:jimm]]

(In reply to comment #19)
> (In reply to comment #18)
> > Might the box object values be off kilter from the 0,0 origin of the drawWindow
> > call, since the y origin of the window is -8?
> 
> When not maximized, the origin appears to be -2. I took screenshots of both
> states and the values of boxObject indicate that it expects the origin's y
> coordinate to be the row with the first orange pixel.

There's some css that sets margin-top to 8px in this case.

http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/browser-aero.css (In reply to comment #22)

> Comment on attachment 460442 [details] [diff] [review]
> The x case isn't actually needed, is it? We don't set horizontal margins on the
> window, and I'm not sure there would ever be a reason to. (If we did, would it
> be -moz-margin-start? Probably not, I guess...)

I think it will be once we implement our own window frame. Content will be sitting at 8x8 in the client area when in the maximized state.

Comment 24

[Jim Mathies [:jimm]]

Noticed the two pixel offset is in there as well. (Not sure when that got added.) 

I think these will need to be updated when the custom frame is implemented, since the frame border width & height will be based on the theming code that hasn't landed yet, and will equal whatever the current user has set for border widths through the advanced window settings properties.

Maybe we should mark this dependent on the framing (bug 575870)?

Comment 25

[Jim Mathies [:jimm]]

I went ahead and changed dependencies. Once we have an actual frame tied to system display prefs, this should be pretty easy to fix up.

Comment 26

[Rob Arnold [:robarnold]]

Attachment: Slight hack v2

Unless we're going to fix that soon, I'd rather get this in for b3.

Comment 27

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 461063 [details] [diff] [review]
Slight hack v2

>diff --git a/browser/components/wintaskbar/WindowsPreviewPerTab.jsm b/browser/components/wintaskbar/WindowsPreviewPerTab.jsm

>   previewTabCallback: function (ctx) {

>+    // domWin is not an actual DOM element

I don't understand the purpose of this comment. Just remove it?

>+    let elem = domWin.document.documentElement;
>+    let style = domWin.getComputedStyle(elem,null);
>+    function extractPixelProperty(prop) {
>+      let prop = style.getPropertyValue(prop);
>+      return parseInt(prop);
>+    }
>+    let x = boxObject.x + extractPixelProperty('-moz-margin-start');
>+    let y = boxObject.y + extractPixelProperty('margin-top');
>+    ctx.translate(x, y);

I don't understand how Jim proposes we fix this after bug 575870 lands, but it sounds like it would mean removing this hack? r=me assuming that's true, and we get a followup filed on removing this.

Comment 28

[Dietrich Ayala (:dietrich)]

Blocking+ for janky-looking primary UI.

Comment 30

[Jim Mathies [:jimm]]

Should be fixed by the landings in bug 575870 & bug 574454.

Comment 31

[Jim Mathies [:jimm]]

looks like this got fixed at some point. resolving wfm.