496371 - Style not showing correctly for incompatible add-on placeholders in featured add-ons box

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Public Pages, defect)

Description

[Dave Garrett]

Attachment: screenshots

View the front page in a recent nightly and you'll see a whole host of "This add-on is for older versions of Firefox" placeholders. Compare the style of the ones in the featured box up top and the recommended and popular panes in the box below. (see attached) The ones below look nice but up top the text is misaligned to the left instead of centered and it's missing the green coloring for recommended status. The "Recommended" label is gone too, but that was probably intentional. Also note that the experimentals up here are missing their red/pink coloring as well (see bug 496364).

Comment 1

[Stephen Donner [:stephend] Not actively reading bugmail]

Can we please 5.0.6 this?  Looking crappy in IE is one thing, but I want to look great in Firefox, at least.

Comment 2

[Wil Clouser [:clouserw]]

Attachment: add flags

This fixes this bug and bug 496364.  The only thing I haven't figured out is the button in RTL is off center, but only in this area - the other buttons on the page are fine.  Seems like it should be obvious but I think I've been up too long.

I'm sure Ehsan or rdoherty will know why

Comment 3

[Wil Clouser [:clouserw]]

Attachment: example of broken rtl button after this patch

Comment 4

[(no longer active)]

Attachment: RTL fix

This patch should fix the RTL problem.  With this rule in main-mozilla.css while the original rule in main.css, it overrides some other rules in main.css for RTL locales.  In this case, the rule which is mistakenly overridden is:

.section-teaser .addon-view p,
.section-teaser .addon-view h3 {
    margin-left: 0;
    margin-right: 0;
    line-height: 1.23em;
}

(the margin-left and margin-right rules are overridden of course).

Comment 5

[Fred Wenzel [:wenzel]]

Comment on attachment 381712 [details] [diff] [review]
add flags

That looks good.

Comment 6

[Fred Wenzel [:wenzel]]

Comment on attachment 381743 [details] [diff] [review]
RTL fix

No, this does not fix the bug unfortunately. The problem is not the rule being overwritten. The problem is the rule itself.

If we need it to stay, so that the add-on descriptions in that box have a proper margin, you probably need to add a rule for

.html-rtl .section-teaser .column p.install-button

and remove the margin-right there.

Comment 7

[(no longer active)]

I tried to apply the effects of Wil's patch inside Firebug and go from there, but I may have easily been misguided.  Maybe we can land this and file a follow-up for the RTL case?  Once it shows up on preview I can prepare a working patch, I'm sure.

Comment 8

[Wil Clouser [:clouserw]]

Thanks wenzel.  This is in r27225.  Leaving open for Ehsan's patch.

Comment 9

[(no longer active)]

Comment on attachment 381743 [details] [diff] [review]
RTL fix

(In reply to comment #6)
> (From update of attachment 381743 [details] [diff] [review])
> No, this does not fix the bug unfortunately. The problem is not the rule being
> overwritten. The problem is the rule itself.

I investigated this deeply, and while I don't have an AMO instance running locally, I really think that my patch should fix this issue...

> If we need it to stay, so that the add-on descriptions in that box have a
> proper margin, you probably need to add a rule for
> 
> .html-rtl .section-teaser .column p.install-button
> 
> and remove the margin-right there.

What is happening is that we have the rule |.section-teaser .column p, .section-teaser .column h3| in main.css which tries to set a left margin to p and h3 elements inside the teaser columns, but then again we have another rule |.section-teaser .addon-view p, .section-teaser .addon-view h3| which AFAICT overrides the original rule in every case, since all .column's are in .addon-view li's.

Now, we have a |.html-rtl .section-teaser .column p, .html-rtl .section-teaser .column h3| after both these rules in main-mozilla.css which tries to reverse the |.section-teaser .column p, .section-teaser .column h3| rule for RTL mode, but also overrides |.section-teaser .addon-view p, .section-teaser .addon-view h3| which sets left/right margins to 0.  This causes the correct zero margins not to take affect.

My patch moves the RTL rule to exactly after the first rule to make it correct for RTL mode (even though the first rule seems useless), and therefore the |.section-teaser .addon-view p, .section-teaser .addon-view h3| should kick in for RTL mode exactly like LTR mode.

Comment 10

[Fred Wenzel [:wenzel]]

Comment on attachment 381743 [details] [diff] [review]
RTL fix

I think what you are missing is that the rule you are moving takes effect all the time, no matter where you put it, as it matches "better" than the non-.html-rtl rule. Here's what fixed it:

diff --git a/site/app/webroot/css/amo2009/main-mozilla.css b/site/app/webroot/css/amo2009/main-mozilla.css
index 17f30ac..359e5e6 100644
--- a/site/app/webroot/css/amo2009/main-mozilla.css
+++ b/site/app/webroot/css/amo2009/main-mozilla.css
@@ -194,6 +194,9 @@ p.install-button {
     margin-left: 0;
     margin-right: 1em;
 }
+.html-rtl .section-teaser .column p.install-button {
+    margin-right: 0;
+}
 .html-rtl .section-teaser .teaser-header ol li {
     margin-left: 5em;
     margin-right: 0;

I'll check that in like this if that's all right with you.

Comment 11

[Fred Wenzel [:wenzel]]

r27249 -- Ehsan, can you verify the bug please, one preview picks it up? Thanks!

Comment 12

[(no longer active)]

(In reply to comment #11)
> r27249 -- Ehsan, can you verify the bug please, one preview picks it up?
> Thanks!

Yes, it's fixed now.  Thanks!

Comment 13

[Stephen Donner [:stephend] Not actively reading bugmail]

This fix made my day, I want you to know.

Verified FIXED.

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

removing "push-needed" from 105 AMO 5.0.6 bugs; filter on "I hate stephend!"