470711 - Newly modernized tabs look cut off if the last one is hidden

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[J Luh]

Attachment: screen shot of problem

The folder properties dialog for local folders is drawn improperly; the rounded corners on the Retention Policy tab do not show. See the screen shot.

To duplicate: right click on Local Folders -> Inbox and select Properties.

Using 3.0b1 on Mac OS X Intel. Sorry if this is a dupe; I searched but did not find anything similar.

Comment 1

[Phil Ringnalda (:philor)]

Markus: thoughts? It's http://mxr.mozilla.org/comm-central/source/mailnews/base/resources/content/folderProps.xul#66 where http://mxr.mozilla.org/comm-central/source/mailnews/base/resources/content/folderProps.js#273 sets .hidden on things including the last tab onload.

Comment 2

[Phil Ringnalda (:philor)]

Though even if we work around it in that dialog, there's still who knows how many extensions doing it (Enigmail was the one I saw a screenshot of this morning), and I don't think we want the solution to be an MDC paragraph saying "even though it looks like it works just fine to set a <tab> to .hidden, don't do it, because it looks like crap on OS X."

Comment 3

[Markus Stange [:mstange]]

The appearance of the tabs is based on the presence of the "first-tab" / "last-tab" attributes. So we should probably update them whenever .hidden changes.

Comment 4

[Markus Stange [:mstange]]

Attachment: tabbox.xml approach, v1

This patch implements what I outlined in the previous comment.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 354569 [details] [diff] [review]
tabbox.xml approach, v1

>+          this.setAttribute("hidden", val ? "true" : "false");

Setting .hidden = false should remove the attribute.

Comment 6

[Dão Gottwald [:dao]]

Also, this won't work for .setAttribute("hidden", "true") or using display:none directly. This could probably be fixed by calling updateVisibilityFlags from the tab constructor and destructor. Or is there away to fix this on the widget level so that the first/last-tab attributes aren't needed at all?

Comment 7

[Markus Stange [:mstange]]

Attachment: nsNativeTheme approach, v1

> Or is there away to fix this on the widget
> level so that the first/last-tab attributes aren't needed at all?

Yeah, and that's even easier. The only disadvantage of this approach is that theme developers who style the tabs with CSS don't benefit from it.

Comment 8

[Josh Aas]

Comment on attachment 354608 [details] [diff] [review]
nsNativeTheme approach, v1

>+  nsIFrame* first = aFrame->GetParent()->GetFirstChild(NULL);

Argument to GetFirstChild should be nsnull.

Comment 9

[Markus Stange [:mstange]]

pushed: http://hg.mozilla.org/mozilla-central/rev/823c957d952e

Comment 10

[Markus Stange [:mstange]]

Landed on 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/50b1f7631625

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We don't have a possibility to test this automatically, right? Can we get a simple testcase which shows the broken behavior? It would be nice to have it in Litmus at least.

Comment 12

[Markus Stange [:mstange]]

It's possible to write a reftest for this. I'll make one.

Comment 13

[Markus Stange [:mstange]]

Attachment: reftest

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090103 Shredder/3.0b2pre ID:20090103025757

Comment 15

[Markus Stange [:mstange]]

Checked in the reftest on both mozilla-central and mozilla-1.9.1.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Just for information:

http://hg.mozilla.org/mozilla-central/rev/e38385c26b20
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c605e0375e2e