470048 - feed and "popup blocked" button cleanup

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch v2

missed another feedButton.setAttribute("tooltiptext", ...)

Comment 2

[Dão Gottwald [:dao]]

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

Comment 3

[Pardal Freudenthal (:ShareBird)]

The changes on #page-report-button break third party themes.

The only way to make a selector which will not conflict with FF 2.0 code is to use. for example, the window attribute "titlemodifier_privatebrowsing" which only exists on FF 3.1 upwards. But this bug was not landed on the branch yet (why?), so, it's impossible to make a rule using it to match all FF versions (if there is one, please, let me know, but preferably not using the chrome.manifest flags, since I already use it for distinct rules for 2.0 downwards and 3.0 upwards. If I need to do that for every version it will be very difficult to manage changes on the theme ).

Comment 4

[Dão Gottwald [:dao]]

Attachment: branch patch

per comment 3

Comment 5

[Dão Gottwald [:dao]]

Gavin: ping? Should be an easy review.

Comment 6

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

I don't really understand how that CSS patch is going to help third party themes. Can you spell it out for me?

Comment 7

[Dão Gottwald [:dao]]

It moves the hiding based on the absence of the "blocked" attribute to browser/base/content/browser.css, which allows (but doesn't force) theme authors to remove the handling of that attribute and use the same styling for 3.1 and above.

Comment 8

[Pardal Freudenthal (:ShareBird)]

This patch doesn't help third party themes. I guess you didin't understand the real issue on it. Sorry if I didn't explain myself well...

The problem is that the trunk build uses the property "hidden" to get rid of the button (and using it visible per default) and the branch (and all versions downwards) uses the "broked" attribute to make it appear. At third party themes side it's possible to define CSS rules to differentiate 3.0 downwards from 3.1 upwards (as I described on comment 3), so the best for us should be to have the same approach on trunk and branch. 

I also don't understand why the patch for the branch must be different from trunk...

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #8)
> This patch doesn't help third party themes. [...] At third party themes
> side it's possible to define CSS rules to differentiate 3.0 downwards from 3.1
> upwards (as I described on comment 3), so the best for us should be to have the
> same approach on trunk and branch. 

Did you test it? The patch does allow you to use the same code for trunk and branch.

Comment 10

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

(In reply to comment #8)
> I also don't understand why the patch for the branch must be different from
> trunk...

Breaking themes to allow simplifying code is acceptable on trunk, since it is far enough away from release that theme authors will have time to adjust. Unless we think that this is major enough that we need to retain compatibility even across major version changes, of course - but I don't think it is.

Comment 11

[Pardal Freudenthal (:ShareBird)]

> Did you test it? The patch does allow you to use the same code for trunk and
> branch.

Sorry. You're right. I've figured out now what I have to do. Sorry again.

Comment 12

[Pardal Freudenthal (:ShareBird)]

Will this patch land for b3?

Comment 13

[Dão Gottwald [:dao]]

Gavin: I repeatedly read that theme authors want this on branch. Do you see any downside to attachment 360369 [details] [diff] [review]?

Comment 14

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

Comment on attachment 360369 [details] [diff] [review]
branch patch

I still don't understand comment 7 or comment 8 (maybe an example would help), but the change itself looks fine, I guess!

Comment 15

[Pardal Freudenthal (:ShareBird)]

Please don't consider comment 8. Dão is right. The patch allows the use of the same code for branch and trunk.

Comment 16

[Dão Gottwald [:dao]]

Simple example, borrowed from our own themes:

Pre-cleanup:

>#page-report-button {
>  display: none;
>}
>
>#page-report-button[blocked] {
>  list-style-image: url("chrome://browser/skin/Info.png");
>  display: -moz-box;
>}

Post-cleanup:

>#page-report-button {
>  list-style-image: url("chrome://browser/skin/Info.png");
>}

With attachment 360369 [details] [diff] [review], both methods will work for 3.1.

Comment 17

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

Ah, so the conflict is that themes couldn't specify an image for the no-attribute #page-report-button on branch, because we relied on themes to implement the hiding behavior when the attribute wasn't there, but they need to specify an image for the no-attribute version on trunk because we use "hidden" there?

Comment 18

[Dão Gottwald [:dao]]

Exactly.

Comment 19

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

Comment on attachment 360369 [details] [diff] [review]
branch patch

a191=beltzner

Comment 20

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8dc099114d94

Comment 21

[Pardal Freudenthal (:ShareBird)]

I'm really sorry having to come back to this bug, but I've just figured out another side effect impacting third party themes. With this patch,the feed button now becomes a "collapsed" attribute if the site has no feeds. If the site has just one feed, it becomes the "feed" attribute. But if a site has more then one feed, it becomes no attributes, making it very the same as sites without feeds on FF 2.0 and FF 3.5. So I'm not seeing a way to make a rule which will fit on FF 3.6 and downwards. Am I missing something?

Comment 22

[Dão Gottwald [:dao]]

The "feed" attribute is a leftover... it should have neither the "feed" nor the "feeds" attribute, just "collapsed".

Comment 23

[Dão Gottwald [:dao]]

And themes shouldn't have to worry about that, as #feed-button:not([feeds]) { visibility: collapse; } was part of browser/base/content/browser.css.

Comment 24

[Pardal Freudenthal (:ShareBird)]

I wrote FF 2.0 but actually was meaning FF 3.0.

(In reply to comment #23)
> And themes shouldn't have to worry about that, as #feed-button:not([feeds]) {
> visibility: collapse; } was part of browser/base/content/browser.css.

Hmmm, I guess it wasn't... It was part of skin: http://mxr.mozilla.org/firefox/source/browser/themes/winstripe/browser/browser.css#1768

Comment 25

[Dão Gottwald [:dao]]

It's in browser/base/content/browser.css on 1.9.1.

Comment 26

[Pardal Freudenthal (:ShareBird)]

(In reply to comment #25)
> It's in browser/base/content/browser.css on 1.9.1.

Yes. I agree there are no really compatibility issues between 3.6 and 3.5, but for 3.0... For themes to work on FF 3.0, this rule have to be inside our browser.css. So, the only way I'm seeing to make the theme compatible with 3.0, 3.5 and 3.6 is using appversion flags on chrome.manifest and I was wanting to avoid this.
Or I am missing something else?

Comment 27

[Dão Gottwald [:dao]]

You could use the hack from comment 3 to distinguish <3.5 from >=3.5. Or you could use the old code for both 3.0 and 3.5, and move to the new code (i.e. drop support for 3.0) when 3.6 becomes interesting.

Comment 28

[Dão Gottwald [:dao]]

(In reply to comment #22)
> The "feed" attribute is a leftover... it should have neither the "feed" nor the
> "feeds" attribute, just "collapsed".

Actually "feed" is not a leftover. It's there because the subscribeToFeed method reads it, not for styling.

Comment 29

[Pardal Freudenthal (:ShareBird)]

(In reply to comment #27)
> You could use the hack from comment 3 to distinguish <3.5 from >=3.5. Or you
> could use the old code for both 3.0 and 3.5, and move to the new code (i.e.
> drop support for 3.0) when 3.6 becomes interesting.

Thank you for taking care of this issue. Very appreciated.

I'm not wanting to drop support for old versions. One of the goals from my theme is compatibility. After your branch patch (attachment 360369 [details] [diff] [review]) I can have just one file fully compatible with FF 1.5 up to 3.6a1pre and TB 1.5 up to 3.1a1pre...

To use the hack (titlemodifier_privatebrowsing), I will need to use a descendant selector, I try to avoid such rules for performance... (Make it sense??)

So, I guess I will make another folder to contain different rules for 3.5 upwards, as I already have for distinguish rules for 2.0 and 3.0. A little bit confuse sometimes but not really a big deal if I register an "only skin" chrome package containing a couple files and importing them into the necessary stylesheets as described at http://forums.mozillazine.org/viewtopic.php?f=18&t=906535&p=4756995#p4756995 (just as information if other authors are running into this issue).

Anyway, thanks.