463217 - New Tab and Overflow Scroll buttons look like they're sitting on a pedestal

Status: VERIFIED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect, P2)

Description

[Jim Jeffery not reading bug-mail 1/2/11]

The 'New tab' button looks like its sitting on a pedestal.  See screenshot here from the Builds Forum:

http://forums.mozillazine.org/viewtopic.php?p=4903505#p4903505

Caused by? https://bugzilla.mozilla.org/show_bug.cgi?id=458487 
or maybe due to the landing of the 'All Tabs Preview' ? 

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081105 Minefield/3.1b2pre Firefox/3.0 ID:20081105032350

Comment 1

[Dão Gottwald [:dao]]

(In reply to comment #0)
> or maybe due to the landing of the 'All Tabs Preview' ?

nope

Comment 2

[IU]

Bug also affects the tab overflow arrows.

Comment 3

[u88484]

Why was the whiteboard cleared of things Alex specifically wants added to visual bugs, especially high visibility bugs?  Also, 'polish' from the keyword?

Comment 4

[Dão Gottwald [:dao]]

Because this isn't a polish bug. It's probably caused by bug 458487 but in any case it's a core bug.

Comment 5

[Markus Stange [:mstange]]

And it might be fixed by bug 456330.

Comment 6

[u88484]

(In reply to comment #4)
> Because this isn't a polish bug. It's probably caused by bug 458487 but in any
> case it's a core bug.

Well by Alex's blog post (http://tinyurl.com/56gc6m) it is.  "[polish-hard] - something that is not trival to fix (like rewriting the focus code)"

And yes at first I thought it was an easy fix, as in changing some css but now it is clear it is not 'polish-easy' but 'polish-hard' so it should still have those keywords in the whiteboard.

I'm just trying to get this on Alex's radar so beta 2 dosen't ship with messed up icons...especially on an icon for a "feature" that was just implemented (new tab button on tabbar).

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't see it on Mac. Could be fixed by patch in bug 456330 or bug 463204.

Comment 8

[Jim Jeffery not reading bug-mail 1/2/11]

Using an hourly build, changeset:
http://hg.mozilla.org/mozilla-central/rev/18fec592b35b
which should contain the patches in comment #7 did not fix the appearance of the Icons.

Comment 9

[John P Baker]

There is something odd with -moz-image-region

.tabs-newtab-button > .toolbarbutton-icon {-moz-image-region: auto}

clears the pedestal for me.

Comment 10

[John P Baker]

(In reply to comment #9)
> There is something odd with -moz-image-region
> 
> .tabs-newtab-button > .toolbarbutton-icon {-moz-image-region: auto}
> 
> clears the pedestal for me.

Is it possible that the code in bug 458487 is tiling the image if it is smaller than the -moz-image-region?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah I think we're tiling the image to fill the -moz-image-region. Oops.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Work in progress. This should fix the bug. I just need to add some comments and the reftest.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v1

Comment 15

[Ryan VanderMeulen [:RyanVM]]

The patch here does fix the icon starting to repeat, but it appears that the icon is still misaligned. It's shifted up relative the height of the adjacent tabs.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, it looked OK to me. Can you modify the testcase in the patch to show the bug?

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: screenshot

This is what I see in my build with the patch --- it looks fine to me. Are you seeing something else?

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Highly visible impact on the Windows beta, should fix before b2.

Comment 19

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

Any chance you could briefly explain the patch?

Comment 20

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

Comment on attachment 347053 [details] [diff] [review]
fix v1

Well, r+sr=dbaron anyway.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If aSourceArea is non-null and references a rectangle that extends outside the image bounds (0,0,imageWidthInAppUnits,imageHeightInAppUnits), we get into a situation where 'fill' extends outside 'dest'. Then the call to DrawImage will trigger tiling, which we don't want.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed 635057569226

Comment 23

[Jim Jeffery not reading bug-mail 1/2/11]

Verified fixed using changeset:

http://hg.mozilla.org/mozilla-central/rev/b5f3b30402cb

Hourly build:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081110 Minefield/3.1b2pre Firefox/3.0 ID:20081110005319

Comment 24

[Alex Faaborg [:faaborg] (Firefox UX)]

>"[polish-hard] - something that is not trival to fix (like rewriting the focus
>code)"

Yeah, this is a good example of [polish-hard] [polish-high-visibility], just so everyone is on the same page with the various whiteboard terms we are trying out.

Comment 25

[Dão Gottwald [:dao]]

(In reply to comment #24)
> >"[polish-hard] - something that is not trival to fix (like rewriting the focus
> >code)"
> 
> Yeah, this is a good example of [polish-hard] [polish-high-visibility], just so
> everyone is on the same page with the various whiteboard terms we are trying
> out.

I assume that status whiteboard messages like [polish-hard] only nail the 'polish' keyword down, but don't fundamentally change its meaning. This bug doesn't meet the criteria for 'polish' (https://bugzilla.mozilla.org/describekeywords.cgi), because it isn't about the UI -- it's a Core:Layout regression that can manifest itself anywhere.