The default bug view has changed. See this FAQ.

Remove identity-box-inner and page-proxy-stack

RESOLVED FIXED in Firefox 16

Status

()

Firefox
Location Bar
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

Trunk
Firefox 16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
time for some cleanup
(Assignee)

Comment 1

5 years ago
Created attachment 630561 [details] [diff] [review]
patch
Attachment #630561 - Flags: review?(mano)
(Assignee)

Updated

5 years ago
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Updated

5 years ago
Attachment #630561 - Flags: review?(jaws)
Comment on attachment 630561 [details] [diff] [review]
patch

Review of attachment 630561 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.xul
@@ +486,5 @@
>                 code fires onmousedown, and hence eats our favicon drag events.
>                 We only add the identity-box button to the tab order when the location bar
>                 has focus, otherwise pressing F6 focuses it instead of the location bar -->
>            <box id="identity-box" role="button"
> +               align="center"

\o/ for simpler selectors, but why do we need to add align="center" here?
(Assignee)

Comment 3

5 years ago
align="center" is moved from #identity-box-inner.
Comment on attachment 630561 [details] [diff] [review]
patch

Review of attachment 630561 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, thanks.  I missed that in the middle of the other XUL changes.

::: browser/base/content/browser.css
@@ +274,5 @@
>  #identity-icon-country-label {
>    direction: ltr;
>  }
>  
> +#identity-box.verifiedIdentity > #identity-icon-labels > #identity-icon-label {

We could probably also remove the |#identity-box| portion of the selector here since it's not really needed, but it has the potential to lower the precedence of the rule. If we removed it, then this file would be more consistent with our theme versions of browser.css since they don't prefix the .verified[Identity,Domain] with #identity-box.
Attachment #630561 - Flags: review?(jaws) → review+
(Assignee)

Comment 5

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/48154e23df93
Target Milestone: --- → Firefox 16
(Assignee)

Updated

5 years ago
Attachment #630561 - Flags: review?(mano)
https://hg.mozilla.org/mozilla-central/rev/48154e23df93
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
I do apologize in advance for commenting in this fixed bug...

There are a couple third-party themes using these two elements for styling the identity box (including my theme). 

I'm using the identity-box-inner to build a real button (using the three images approach). I know I could use the border-image approach but I see a disadvantage with this method, since I can't bring all images I need into a single file. I'm using the hack to preload the images, but this method seems to fail sometimes.

Another problem I see is that every theme using a background for this box will need the glow behind the icons. Without the page-proxy-stack element I can't see any possibility to style the box properly.

Notice that we are already able to use these elements in FF14 and FF15. It will be very frustrating if we have to change the whole approach for FF16. My goal and the goal from other developers is to maintain appearance consistence across several versions from the applications.

Could you please consider backing this bug out for the trunk?
(Assignee)

Comment 8

5 years ago
(In reply to Pardal Freudenthal (:ShareBird) from comment #7)
> Another problem I see is that every theme using a background for this box
> will need the glow behind the icons.

What icons?
(In reply to Dão Gottwald [:dao] from comment #8)
> (In reply to Pardal Freudenthal (:ShareBird) from comment #7)
> > Another problem I see is that every theme using a background for this box
> > will need the glow behind the icons.
> 
> What icons?

I mean the identity icons. I think that themes using a colored background for the box will run into the same problem as discussed in Bug 430767 (where the glow image was introduced to better distinguish favicons).
(Assignee)

Comment 10

5 years ago
Identity icons are completely under the control of the theme... they should be designed with the background color of the identity box in mind.
(In reply to Dão Gottwald [:dao] from comment #10)
> Identity icons are completely under the control of the theme... they should
> be designed with the background color of the identity box in mind.

Sorry for the long time I've needed to answer here. I'm very busy at work, I had no time to look at it.
Yes, you are right... I will try to implement it using border-image and changing the icons when I manage some time to work on it.

Anyway, will this be in Firefox 15.0?
This will be in Firefox 16.
(In reply to Jared Wein [:jaws] from comment #12)
> This will be in Firefox 16.

This is bad... It means I need to implement it differently for Firefox 13.0 and backwards (they use the favicon with glow), 14.0 (the new identity buttons), 15.0 (because border-image is implemented differently for 15.0 upwards) and for 16.0 upwards (without the box for the glow). And the only way to do this is using appversion manifest flags and splitting the code. 
All this amount of work just because a simple patch...

Another issue I can see. Although we have control how the identity buttons can look, we have no control about favicons (the reason the glow was introduced at all). I guess there are already a couple extensions bringing the favicons back to the identity box. This will cause problems for themes using background colors on it...
(Assignee)

Comment 14

5 years ago
(In reply to Pardal Freudenthal (:ShareBird) from comment #13)
> (In reply to Jared Wein [:jaws] from comment #12)
> > This will be in Firefox 16.
> 
> This is bad... It means I need to implement it differently for Firefox 13.0
> and backwards (they use the favicon with glow), 14.0 (the new identity
> buttons), 15.0 (because border-image is implemented differently for 15.0
> upwards) and for 16.0 upwards (without the box for the glow). And the only
> way to do this is using appversion manifest flags and splitting the code. 
> All this amount of work just because a simple patch...

Firefox 13-15 will be unsupported and insecure once Firefox 16 is released. It seems unreasonable to support these obsolete Firefox versions in a theme update for Firefox 16. Those who installed your theme in Firefox 13, 14 or 15 and haven't updated to Firefox 16 yet can just keep that theme version and get the latest theme update along with the Firefox 16 update.

> Another issue I can see. Although we have control how the identity buttons
> can look, we have no control about favicons (the reason the glow was
> introduced at all). I guess there are already a couple extensions bringing
> the favicons back to the identity box. This will cause problems for themes
> using background colors on it...

Dealing with that is up to these extensions.
You need to log in before you can comment on or make changes to this bug.