349196 - All Tabs menu doesn't use right edge image with browser.tabs.closeButtons = 3

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[hhh]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060817 BonEcho/2.0b1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060817 BonEcho/2.0b1

alltabs-box-start-bkgnd.png seems to be getting chopped of on Windows XP using either the Luna or Classic OS themes.

Reproducible: Always

Steps to Reproduce:
1. Open a few tabs.
2. View the All Tabs button.
3.

Actual Results:  
The button appears to be chopped off.

Expected Results:  
The button should have a rounded right-corner and a clearly defined border.

Comment 1

[Peter Kasting]

Can you give a screenshot of this?  Specifically, I'm wondering if this is for the "tab close button to right of all tabs menu" case.

Comment 2

[hhh]

Attachment: alltabs

Comment 3

[hhh]

Attachment: alltabs with tab overflow

Comment 4

[hhh]

Attachment: alltabs:hover w/ browser.tabs.closeButtons @ 3

Comment 5

[hhh]

Sorry, that first pic isn't cropped right, but the other two should show it. In global/icons of classic.jar there is the following image which doesn't seem to be getting displayed...

Comment 6

[hhh]

Attachment: alltabs-right

Comment 7

[Peter Kasting]

OK, so looking at your screenshots, this is indeed for the "closeButtons = 3" case.

Changing summary in hopes of being clearer, requesting blocking.

Comment 8

[Adam Kowalczyk]

*** Bug 352356 has been marked as a duplicate of this bug. ***

Comment 9

[Simon Bünzli]

Dao: Could you also have a look at this, once you're done with bug 352321?

Comment 10

[Dão Gottwald [:dao]]

One solution would be to add a right border to the alltabs button, which gets cropped off when the button is the :last-child.

(In reply to comment #6)
> Created an attachment (id=234487) [edit]
> alltabs-right

That's for RTL mode, I guess.

Comment 11

[Simon Bünzli]

(In reply to comment #10)
Why not just use tab-middle-bkgnd.png as a background for the close button's container (and maybe add a 1px separator to the close button's container)? This should be a cheap fix which makes the close button look like part of the whole tab bar concept by only modifying the close button0s appearance itself (less regression risks).

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #11)
Yes, that would work. Images coming.
Update to browser/themes/winstripe/browser/browser.css:

.tabs-closebutton {
  list-style-image: url("chrome://global/skin/icons/close.png");
  -moz-appearance: none;
  -moz-image-region: rect(0px, 16px, 16px, 0px);
  padding: 4px 2px 3px 2px;
  margin-bottom: 1px;
  border: none !important;
  background: -moz-dialog url("chrome://global/skin/icons/close-bkgnd.png");
}

.tabs-closebutton:hover {
  -moz-image-region: rect(0px, 32px, 16px, 16px);
  background-image: url("chrome://global/skin/icons/close-bkgnd-hover.png");
}

Comment 13

[Dão Gottwald [:dao]]

Attachment: close-bkgnd.png

Comment 14

[Dão Gottwald [:dao]]

Attachment: close-bkgnd-hover.png

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #12)
>   padding: 4px 2px 3px 2px;

better:

>   padding: 4px 2px 3px;

Now somebody would have to create the patch.
Seth? :)

Comment 16

[Dão Gottwald [:dao]]

Attachment: screenshot

Comment 17

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Sidebar misapply

Unless I'm mistaken the CSS as it is now will also affect the Sidebar close buttons as in the attachment.  Adding a parent selector (.tabs-closebutton-box > ) to the rules seems to fix the problem (and is how I did it in my Stylish fix 
http://userstyles.org/style/show/1063).  I think this would make the stylesheet something like:

.tabs-closebutton {
  list-style-image: url("chrome://global/skin/icons/close.png");
  -moz-appearance: none;
  -moz-image-region: rect(0px, 16px, 16px, 0px);
  padding: 4px 2px 3px 2px;
  margin-bottom: 1px;
  border: none !important;
}

.tabs-closebutton-box > .tabs-closebutton {
  background: -moz-dialog url("chrome://global/skin/icons/close-bkgnd.png");
}

.tabs-closebutton:hover {
  -moz-image-region: rect(0px, 32px, 16px, 16px);
}

.tabs-closebutton-box > .tabs-closebutton:hover {
  background-image: url("chrome://global/skin/icons/close-bkgnd-hover.png");
}

Comment 18

[Dão Gottwald [:dao]]

(In reply to comment #17)
> Unless I'm mistaken the CSS as it is now will also affect the Sidebar close
> buttons as in the attachment.

You're right. I don't understand why the class is named .tabs-closebutton, though. That's misleading.

Comment 19

[Simon Bünzli]

*** Bug 357165 has been marked as a duplicate of this bug. ***

Comment 20

[Hannes Verschore [:h4writer]]

*** Bug 357698 has been marked as a duplicate of this bug. ***

Comment 21

[Daniel Veditz [:dveditz]]

Polish is nice, but not blocking 1.8.1.1: moving to the "wanted" list.

Comment 22

[Dão Gottwald [:dao]]

Attachment: patch

Comment 23

[Dão Gottwald [:dao]]

Attachment: close-bkgnd.png

Comment 24

[Dão Gottwald [:dao]]

Attachment: screenshot

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 260500 [details] [diff] [review]
patch

I'll attach a RTL compatible patch later

Comment 26

[Dão Gottwald [:dao]]

Attachment: close-bkgnd-rtl.png

Comment 27

[Dão Gottwald [:dao]]

Attachment: patch (part 1)

Comment 28

[Robert Strong (they/them - no direct email)]

Comment on attachment 260568 [details] [diff] [review]
patch (part 1)

You need to add these files to the jar.mn as well.

Comment 29

[Dão Gottwald [:dao]]

Attachment: patch (part 2)

Comment 30

[Dão Gottwald [:dao]]

Attachment: synced with trunk

would somebody please check this in?

Comment 31

[Dave Townsend [:mossop]]

Checking in browser/base/content/tabbrowser.xml;
/cvsroot/mozilla/browser/base/content/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.239; previous revision: 1.238
done
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.css
new revision: 1.77; previous revision: 1.76
done
Checking in browser/themes/winstripe/browser/jar.mn;
/cvsroot/mozilla/browser/themes/winstripe/browser/jar.mn,v  <--  jar.mn
new revision: 1.42; previous revision: 1.41
done
Checking in browser/themes/winstripe/browser/tabbrowser/tabbrowserBindings.xml;
/cvsroot/mozilla/browser/themes/winstripe/browser/tabbrowser/tabbrowserBindings.xml,v  <--  tabbrowserBindings.xml
new revision: 1.13; previous revision: 1.12
done

Comment 32

[Dave Townsend [:mossop]]

Missed image files

RCS file: /cvsroot/mozilla/browser/themes/winstripe/browser/tabbrowser/close-bkgnd-rtl.png,v
done
Checking in close-bkgnd-rtl.png;
/cvsroot/mozilla/browser/themes/winstripe/browser/tabbrowser/close-bkgnd-rtl.png,v  <--  close-bkgnd-rtl.png
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/browser/themes/winstripe/browser/tabbrowser/close-bkgnd.png,v
done
Checking in close-bkgnd.png;
/cvsroot/mozilla/browser/themes/winstripe/browser/tabbrowser/close-bkgnd.png,v  <--  close-bkgnd.png
initial revision: 1.1
done