590931 - URL not greyed out in read-only location bar

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Christian Ascheberg]

User-Agent:       Mozilla/5.0 (Windows NT 6.0; rv:2.0b5pre) Gecko/20100826 Minefield/4.0b5pre
Build Identifier: Mozilla/5.0 (Windows NT 6.0; rv:2.0b5pre) Gecko/20100826 Minefield/4.0b5pre

On my Windows Vista machine, the URL is not greyed out in a popup with read-only location bar. This is working for me with Windows 7 though.

Reproducible: Always

Steps to Reproduce:
1. open popup with read-only location bar
Actual Results:  
URL not greyed out in location bar

Expected Results:  
URL should be greyed out

Comment 1

[Christian Ascheberg]

Attachment: screenshot

Comment 2

[Christian Ascheberg]

I think I have to make some corrections. In my Win7, Aero is not enabled, so the display of the location bar is the same as it was in Firefox 3.6.

So the difference is:
- Vista: background of location bar is only white when it is hovered or has focus. Otherwise its slightly blue. This is also true for popups (with read-only location bar).
- Firefox 3.6 / no Aero: background of the location bar is always white. In popups the location bar is greyed out if it is read-only.

So I think the hover-effect should be disabled for read-only location bar, background should be grey.

Comment 3

[Christian Ascheberg]

Attachment: css patch

what this patch is supposed to do:
- with no personas used: background-color of read-only urlbar is always grey
- with personas used: background-color of read-only urlbar does not change on hover or focus

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 512839 [details] [diff] [review]
css patch

>   #urlbar,
>   .searchbar-textbox {
>-    background-color: rgba(255,255,255,.725);
>     @navbarTextboxCustomBorder@
>     color: black;
>   }
>+  
>+  #urlbar:not([readonly="true"]),
>+  .searchbar-textbox {
>+    background-color: rgba(255,255,255,.725);
>+  }

color: black should be in the second rule as well.

nit: you can use [readonly] instead of [readonly="true"] and [focused] instead of [focused="true"]

Comment 5

[Christian Ascheberg]

(In reply to comment #4)
Thank you for your comment!

> color: black should be in the second rule as well.

I thought the first rule would always apply, so why does it have to be in the second rule then? (Actually, I wondered why it has to be declared here explicitly at all.)

> nit: you can use [readonly] instead of [readonly="true"] and [focused] instead
> of [focused="true"]

Well, that should be easy. What is next then? (After uploading a new patch?)

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #5)
> (In reply to comment #4)
> I thought the first rule would always apply, so why does it have to be in the
> second rule then? (Actually, I wondered why it has to be declared here
> explicitly at all.)

Because it's linked to the background color. For readonly, the background is -moz-dialog, so the text color needs to be -moz-dialogText (which may or may not be black depending on the OS theme).

> Well, that should be easy. What is next then? (After uploading a new patch?)

You set the review flag on the patch to "?" and enter my e-mail address in the requestee field.

Comment 7

[Christian Ascheberg]

(In reply to comment #6)
> Because it's linked to the background color. For readonly, the background is
> -moz-dialog, so the text color needs to be -moz-dialogText (which may or may
> not be black depending on the OS theme).

I don't think I understood that.
1) In the case that the color needs to be hard-coded, I think the given solution should be ok, as the first rule should always apply, thus always setting the color black.
2) In the case that there needs to be a differentiation between whether it is read-only or not, is it necessary to overwrite the settings from chrome://global/skin/textbox.css at all?
I muddled through this using the DOM Inspector.

Thank you for your help!

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #7)
> 2) In the case that there needs to be a differentiation between whether it is
> read-only or not, is it necessary to overwrite the settings from
> chrome://global/skin/textbox.css at all?

No, you shouldn't override the color in that case.

Comment 9

[Christian Ascheberg]

Attachment: css patch v2

What this new patch does for readonly urlbar:
- it removes hover and focus background-color effect
- it inherits -moz-Dialog as background and -moz-DialogText as color, but only if there is no personas applied, because I think it looks odd if there is one.

In the other case (urlbar not readonly or persona applied), background and color are still both set explicitly, just as it was before.

If you removed color:black, color:-moz-FieldText would be inherited then, but I am still not sure if that is wanted (for example because the background can not be inherited).

Comment 10

[Dão Gottwald [:dao]]

(In reply to comment #9)
> - it inherits -moz-Dialog as background and -moz-DialogText as color, but only
> if there is no personas applied, because I think it looks odd if there is one.

how about:

#urlbar[readonly]:-moz-lwtheme {
  opacity: 0.8;
}

Comment 11

[Christian Ascheberg]

(In reply to comment #10)
> how about:
> 
> #urlbar[readonly]:-moz-lwtheme {
>   opacity: 0.8;
> }

setting opacity on the urlbar has an influence on all the items in it: bookmark-star, website-icon, text
I guess only adding new rules setting the background color can fix that.

Something else:
I could add :not([focused]) to two rules, so that a focused readonly urlbar with personas is also grey, like in Firefox 3.6

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #11)
> setting opacity on the urlbar has an influence on all the items in it:
> bookmark-star, website-icon, text

That's ok.

Comment 13

[Christian Ascheberg]

Attachment: current vs. opacity

(In reply to comment #12)

To be honest, I do not like it, especially because of the transparent identity box and text selection.

Wouldn't it be better to keep the current "less-risky" version? It is still an improvement.

Comment 14

[Dão Gottwald [:dao]]

That screenshot doesn't show a readonly url bar, does it?

Comment 15

[Christian Ascheberg]

(In reply to comment #14)
> That screenshot doesn't show a readonly url bar, does it?

Uhm, it does. It's from the given testcase but with another URL loaded. The personas is "Live With Music".
I forgot the labeling though:
left: current status
middle and right: opacity 0.8 with background -moz-Dialog

I am using Win Vista. Any difference for you?

Comment 16

[Christian Ascheberg]

Attachment: css patch v2.5

added: readonly urlbar always grey on focus

Comment 17

[Christian Ascheberg]

Attachment: css patch w/ opacity

sorry for the trouble, I will now stop uploading patches before getting feedback

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 516011 [details] [diff] [review]
css patch w/ opacity

>+#urlbar:-moz-lwtheme:not([focused]),
>+.searchbar-textbox:-moz-lwtheme:not([focused]) {
>+  opacity: .85;
> }

I think we want this only in the readonly case...

Comment 19

[Christian Ascheberg]

(In reply to Dão Gottwald [:dao] from comment #18)
> Comment on attachment 516011 [details] [diff] [review]
> css patch w/ opacity
> 
> >+#urlbar:-moz-lwtheme:not([focused]),
> >+.searchbar-textbox:-moz-lwtheme:not([focused]) {
> >+  opacity: .85;
> > }
> 
> I think we want this only in the readonly case...

Well, the patch is about one year old, but I do not understand that exact comment. The current browser.css includes this:
> #urlbar:-moz-lwtheme,
> .searchbar-textbox:-moz-lwtheme {
>   background-color: rgba(255,255,255,.8);
>   @navbarTextboxCustomBorder@
>   color: black;
> }
So the effect for opacity is the same as with the css above.

Still, overall I am not sure if I understand what the desired design should look like.
For example, the changes in bug 692537 seem to have removed the hover effects in the urlbar with active -moz-lwtheme, and I wonder if that is really better.
Is there any comprehensive documentation about the desired behaviour?

Comment 20

[Christian Ascheberg]

Attachment: patch

Dao, I think I did not understand some of your previous comments correctly, sorry.
I am still interested in providing a patch for this minor bug.
Some parts of the current CSS in question have changed, so this new patch is somewhat easier.

This patch sets opacity on the urlbar in case there is a personas applied. This is already done the same way on Linux and Mac OS. Please tell me if it is unwanted for Windows. The change is slightly visible when looking at e.g. the bookmark star, and I found it is a positive change.
Other than that the patch only removes the hover effect for a readonly urlbar on windows with a (non-default) tabs-on-bottom config.

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 729092 [details] [diff] [review]
patch

(In reply to Christian Ascheberg from comment #20)
> This patch sets opacity on the urlbar in case there is a personas applied.
> This is already done the same way on Linux and Mac OS. Please tell me if it
> is unwanted for Windows. The change is slightly visible when looking at e.g.
> the bookmark star, and I found it is a positive change.

Just changing the background-color is preferable for better legibility. Reducing the opacity fades the text and all other content in the location bar.

Comment 22

[Christian Ascheberg]

Attachment: patch B

Ok, so this patch only changes the background-color, as in the original CSS, but only if it is not a readonly urlbar.

Comment 23

[Christian Ascheberg]

Attachment: comparison

Comparison of patch A and patch B (shows a regular urlbar). The appearance of the urlbar in patch B is exactly the same as in the current nightly.
Top is patch A, below patch B, then again with a different theme.
I still think patch A does not look worse.

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 729611 [details] [diff] [review]
patch B

>--- a/browser/themes/windows/browser.css
>+++ b/browser/themes/windows/browser.css
>@@ -1158,8 +1158,12 @@ toolbar[mode=full] .toolbarbutton-1 > .t
> 
> #urlbar:-moz-lwtheme,
> .searchbar-textbox:-moz-lwtheme {
>+  @navbarTextboxCustomBorder@
>+}
>+
>+#urlbar:-moz-lwtheme:not([focused]):not([readonly]),
>+.searchbar-textbox:-moz-lwtheme:not([focused]):not([readonly]) {
>   background-color: rgba(255,255,255,.8);
>-  @navbarTextboxCustomBorder@
>   color: black;
> }
> 
>@@ -1220,11 +1224,6 @@ html|*.urlbar-input:-moz-lwtheme::-moz-p
>   color: #777;
> }
> 
>-#urlbar:-moz-lwtheme[focused="true"],
>-.searchbar-textbox:-moz-lwtheme[focused="true"] {
>-  background-color: white;
>-}

This causes the native -moz-field color to be used in the background when focusing the textbox with a lightweight theme applied. E.g. when using a high-contrast OS theme, the textbox becomes black on focus.

Comment 25

[Christian Ascheberg]

(In reply to Dão Gottwald [:dao] from comment #24)
> This causes the native -moz-field color to be used in the background when
> focusing the textbox with a lightweight theme applied. E.g. when using a
> high-contrast OS theme, the textbox becomes black on focus.

I have to admit that I did not explicitly think about that before you said it, but I don't see why that is wrong.
Note that with this patch the text color is also adjusted according to the OS theme, so the appearance of a focused urlbar is just as desired by the user.
I'd think that it is counterproductive for such a user if personas prevent the use of the high-contrast theme, especially if it involves a focused textbox.

Comment 26

[Dão Gottwald [:dao]]

(In reply to Christian Ascheberg from comment #25)
> I have to admit that I did not explicitly think about that before you said
> it, but I don't see why that is wrong.

Because going from rgba(255,255,255,.8) to black on focus is jarring.

Comment 27

[Christian Ascheberg]

(In reply to Dão Gottwald [:dao] from comment #26)
> Because going from rgba(255,255,255,.8) to black on focus is jarring.

Ah, I see. Hmm if you want to fully support high-contrast themes etc. and keep a nice transition between non-focused and focused, then the easiest (and only?) general solution I can currently think of is opacity as in the first patch. What do you think? Did you have a look at the attached comparison?

Comment 28

[Dão Gottwald [:dao]]

Just keep the white. We're intentionally abandoning the native color when using a lightweight theme.

Comment 29

[Christian Ascheberg]

Attachment: patch C

(In reply to Dão Gottwald [:dao] from comment #28)
> We're intentionally abandoning the native color when using a lightweight theme.

Ok. May I ask why you do this (also with respect to high-contrast mode)? Should there be something similar in the readonly case?

What you said in comment 26 also applies to the tabsontop=false case, so I kept the "background-color: white" rule there, too.

Comment 30

[Christian Ascheberg]

Attachment: bug-590931-fix.patch

The Australis design already fixed the non-lwtheme part of this bug.
I think this very easy patch now gives a good indication when there is a readonly LWT urlbar (especially in connection with the patch for bug 735607).

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/d8f504ab1f3b

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d8f504ab1f3b