Closed Bug 470048 Opened 13 years ago Closed 13 years ago

feed and "popup blocked" button cleanup

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3.6a1

People

(Reporter: dao, Assigned: dao)

Details

(Keywords: fixed1.9.1)

Attachments

(2 files, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
No description provided.
Attachment #353499 - Flags: review?(gavin.sharp)
Attached patch patch v2Splinter Review
missed another feedButton.setAttribute("tooltiptext", ...)
Attachment #353499 - Attachment is obsolete: true
Attachment #353503 - Flags: review?(gavin.sharp)
Attachment #353499 - Flags: review?(gavin.sharp)
Attachment #353503 - Flags: review?(gavin.sharp) → review+
http://hg.mozilla.org/mozilla-central/rev/d2c8c9456549
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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 ).
Attached patch branch patchSplinter Review
per comment 3
Attachment #360369 - Flags: review?(gavin.sharp)
Gavin: ping? Should be an easy review.
I don't really understand how that CSS patch is going to help third party themes. Can you spell it out for me?
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.
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...
(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.
(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.
> 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.
Will this patch land for b3?
Gavin: I repeatedly read that theme authors want this on branch. Do you see any downside to attachment 360369 [details] [diff] [review]?
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!
Attachment #360369 - Flags: review?(gavin.sharp) → review+
Please don't consider comment 8. Dão is right. The patch allows the use of the same code for branch and trunk.
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.
Attachment #360369 - Flags: approval1.9.1?
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?
Exactly.
Comment on attachment 360369 [details] [diff] [review]
branch patch

a191=beltzner
Attachment #360369 - Flags: approval1.9.1? → approval1.9.1+
Keywords: checkin-needed
Whiteboard: [checkin-needed on 1.9.1]
Keywords: checkin-needed
Whiteboard: [checkin-needed on 1.9.1]
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?
The "feed" attribute is a leftover... it should have neither the "feed" nor the "feeds" attribute, just "collapsed".
And themes shouldn't have to worry about that, as #feed-button:not([feeds]) { visibility: collapse; } was part of browser/base/content/browser.css.
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
It's in browser/base/content/browser.css on 1.9.1.
(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?
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.
(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.
(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.
You need to log in before you can comment on or make changes to this bug.