Last Comment Bug 759299 - Remove identity-box-inner and page-proxy-stack
: Remove identity-box-inner and page-proxy-stack
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Location Bar (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 16
Assigned To: Dão Gottwald [:dao]
:
: Marco Bonardo [::mak]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-29 05:27 PDT by Dão Gottwald [:dao]
Modified: 2012-08-20 16:36 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (9.05 KB, patch)
2012-06-06 07:27 PDT, Dão Gottwald [:dao]
jaws: review+
Details | Diff | Splinter Review

Description Dão Gottwald [:dao] 2012-05-29 05:27:08 PDT
time for some cleanup
Comment 1 Dão Gottwald [:dao] 2012-06-06 07:27:21 PDT
Created attachment 630561 [details] [diff] [review]
patch
Comment 2 Jared Wein [:jaws] (please needinfo? me) 2012-06-12 15:36:04 PDT
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?
Comment 3 Dão Gottwald [:dao] 2012-06-12 15:38:45 PDT
align="center" is moved from #identity-box-inner.
Comment 4 Jared Wein [:jaws] (please needinfo? me) 2012-06-12 16:01:32 PDT
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.
Comment 6 Matt Brubeck (:mbrubeck) 2012-06-13 13:29:29 PDT
https://hg.mozilla.org/mozilla-central/rev/48154e23df93
Comment 7 Pardal Freudenthal (:ShareBird) 2012-08-05 12:15:52 PDT
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?
Comment 8 Dão Gottwald [:dao] 2012-08-10 09:38:18 PDT
(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?
Comment 9 Pardal Freudenthal (:ShareBird) 2012-08-10 12:38:29 PDT
(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).
Comment 10 Dão Gottwald [:dao] 2012-08-10 12:46:35 PDT
Identity icons are completely under the control of the theme... they should be designed with the background color of the identity box in mind.
Comment 11 Pardal Freudenthal (:ShareBird) 2012-08-20 14:34:02 PDT
(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?
Comment 12 Jared Wein [:jaws] (please needinfo? me) 2012-08-20 14:36:57 PDT
This will be in Firefox 16.
Comment 13 Pardal Freudenthal (:ShareBird) 2012-08-20 16:15:42 PDT
(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...
Comment 14 Dão Gottwald [:dao] 2012-08-20 16:36:38 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.