1000625 - Split up general in-content styles from in-content preferences.css

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Valentin Tsatskin [:vt]]

Currently we've dumped all our styles that affect controls into browser/themes/shared/incontentprefs/preferences.css. Although this works for now, if our goal is to have a unified visual style across all in-content pages, we will need to extract these into their own style sheet for embedding into other pages.

My proposal is to move as many of the general styles and resources into browser/themes/shared/in-content.

Comment 1

[Valentin Tsatskin [:vt]]

Attachment: moulting.patch

Since this is related to project Chameleon, I've chosen to name this patch moulting. Moulting is a synonym for shedding, and since Chameleons shed, we can think of this as shedding our styles and resources from preferences and into their own place.

Patch notes:
- Rogue whitespace is removed from preferences.xul
- General image resources have been moved to themes/shared/in-content
- Common.css introduced as a general stylesheet for all shared visual elements in both XUL and HTML about pages.
  - The default namespace is chosen to be HTML instead of the usual XUL namespace to encourage moving away from XUL in-content pages.
  - HTML styles have been somewhat ironed out in https://antlam.github.io/fx-branded/ but not included in this patch for ease of review.

I hoping this gets in before any bitrott and other work happens in about:preferences, that way we can start working with this new structure sooner rather than later.

Comment 2

[Valentin Tsatskin [:vt]]

Attachment: moulting.patch

Same patch notes apply for this one as the previous patch, this is just a bitrott update.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8411485 [details] [diff] [review]
moulting.patch

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

Bitrot... bitrot everywhere :( Given how many conflicts there were trying to apply this patch, probably best for me to wait until you've updated this for me to look at it in depth.

Comment 4

[Asaf Romano (gone)]

Note we also have http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/inContentUI.css which is used in some about: pages. You may want to merge common.css with that file.

Comment 5

[Tim Nguyen :ntim]

(In reply to Mano (needinfo? for any questions; not reading general bugmail) from comment #4)
> Note we also have
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/
> inContentUI.css which is used in some about: pages. You may want to merge
> common.css with that file.

Yeah, but it needs the add-on manager to use the same styling.

Comment 6

[Valentin Tsatskin [:vt]]

(In reply to Tim Nguyen [:ntim] from comment #5)
> (In reply to Mano (needinfo? for any questions; not reading general bugmail)
> from comment #4)
> > Note we also have
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/
> > inContentUI.css which is used in some about: pages. You may want to merge
> > common.css with that file.
> 
> Yeah, but it needs the add-on manager to use the same styling.

I've been looking at inContentUI.css. My plan was to start building out common.css while inContentUI.css still existed in parallel. Once common.css is mature enough, work would be done to replace usages of inContentUI.css with common.css. This might also call for some DOM changes as well.

The rationale here is that older styled pages keep looking the way they currently do without side effects caused from directly working with inContentUI.css while the new visual style is implemented.

Comment 7

[Valentin Tsatskin [:vt]]

Attachment: moulting.patch

A new version that should be free of bitrott. The motivation and ideas carry
over from the previous patch submitted to the bug.


# netError changes

In order to keep the visual styles somewhat consistent between network errors
and about:preferences, I've had to update the body and logo colours to match
those in about:preferences.

The padding required for the dotted line on button focus states was moved over
to netError.css. An updated focus state from Project Chameleon will be added
later in time.


# Additional properties

During the merge, some of the styles from the network error work were carried
over to about:preferences.

* Add pointer cursor to button hover 
  * xul|button:not([disabled="true"]):hover
* Add not-allowed cursor to button hover
  * button[disabled="true"]


# Inconsistencies

body and xul|page use different fonts, message-box and Clear Sans respectively.
This should be addressed in the Project Chameleon overhaul.


# Other

- browser/components/preferences/in-content/preferences.xul includes whitespace
fixes


# Post-discussion

The following discussion shouldn't keep the patch from landing, however I think
it should be addressed.

We are including global.css in the in-content version of preferences.xul.
However, we are essentially reseting these theme specific styles in the theme
specific versions of common.css and preferences.css (see each
<theme>/in-content/common.css and
<theme>/preferences/in-content/preferences.css).

If the motiviation is to have a consistent visual style for in-content pages,
regardless of the platform, why are we even bothering including global.css
just to have to do more work to reset the styles?

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8431224 [details] [diff] [review]
moulting.patch

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

This patch broke the styling of the in-content preferences on Windows8. I glossed over the changes and nothing jumped out at me as to the cause. I'll attach a screenshot

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Screenshot of attachment 8431224

Comment 10

[Valentin Tsatskin [:vt]]

Comment on attachment 8431224 [details] [diff] [review]
moulting.patch

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

::: browser/themes/windows/jar.mn
@@ +425,5 @@
>          skin/classic/aero/browser/identity-icons-https.png
>          skin/classic/aero/browser/identity-icons-https-ev.png
>          skin/classic/aero/browser/identity-icons-https-mixed-active.png
>          skin/classic/aero/browser/identity-icons-https-mixed-display.png
> +        skin/classic/aero/browser/in-content/common.css                (../shared/in-content/common.css)

Looks like I issue jaws noticed on Windows 8 would be right here.

Comment 11

[Valentin Tsatskin [:vt]]

Attachment: moulting.patch

This might fix the problem described in attachment 8431224 [details] [diff] [review]. I've updated the line I commented on earlier to reference the correct resource.

I don't have a windows build environment so I can't really test this, so it's just a wild guess at this point.

Comment 12

[Valentin Tsatskin [:vt]]

(In reply to Valentin Tsatskin [:vt] from comment #11)
> Created attachment 8432577 [details] [diff] [review]
> moulting.patch
> 
> This might fix the problem described in attachment 8431224 [details] [diff] [review]

Meant to reference attachment 8431609 [details].

Comment 13

[Tim Nguyen :ntim]

It'd be better if you put the common.css file into the themes/shared/in-content directory, it'll avoid code duplication. Since project chameleon unifies things across platforms, I see no reason not doing it.

Comment 14

[Tim Nguyen :ntim]

And if you have platform specific issues, you can always use CSS preprocessors.

Comment 15

[Valentin Tsatskin [:vt]]

(In reply to Tim Nguyen [:ntim] from comment #13)
> It'd be better if you put the common.css file into the
> themes/shared/in-content directory, it'll avoid code duplication. Since
> project chameleon unifies things across platforms, I see no reason not doing
> it.

Currently the bulk of the code is in the themes/shared/in-content/common.css. Only platform specific overrides are in themes/<theme>/in-content/common.css, which use preprocessor to include the shared common.css.

(In reply to Tim Nguyen [:ntim] from comment #14)
> And if you have platform specific issues, you can always use CSS
> preprocessors.

Are you suggesting that we merge all of the themes/<theme>/in-content/common.css into themes/shared/in-content/common.css and use %ifdef preprocessors similar to http://hg.mozilla.org/mozilla-central/annotate/6a984e21c2ca/browser/base/content/browser.css#l33?

I like this approach as it would simplify the manifests and reduce the number of files we have. However, are there any drawbacks you can think of here?

Is there a reason we chose the existing structure when dealing with in-content preferences.css (which is the pattern I copied for this patch) instead of your proposed structure? I'm setting needinfo to Jaws to see if he has insight into this.

Maybe all this theme specific work is not required if we don't include global.css. See "Post-discussion" in comment 7 for my thoughts and questions on this.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

We have a couple options and we make the decision based on each situation. If the themes are vastly different, then separate files in each theme directory makes more sense. If the theme only needs slight tweaking per platform, then preprocessors inside of the shared theme file is better. If each theme needs to be adjusted similarly but by different amounts, then make a %define in the per-theme files and make sure to declare it before the shared file is included.

We started the in-content preferences project before we had the shared themes folder, so that explains why it is set up that way. Given how similar each theme displays the in-content prefs, moving to a single file within the /shared/ directory would be an improvement.

Comment 17

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> If the theme only needs slight tweaking per
> platform, then preprocessors inside of the shared theme file is better.

There's no clear rule when to ifdef stuff and when it would be too much, so this is bound to lead to inconsistencies and an overall messier code base. We should just not use platform-ifdefs in shared/ and keep platform-specific things in their dedicated folder.

(In reply to Valentin Tsatskin [:vt] from comment #15)
> I like this approach as it would simplify the manifests

I'm not worried about the complexity of manifest files. They rank among the simplest pieces of code we maintain.

> and reduce the number of files we have.

By itself this isn't really a goal.

Comment 18

[Tim Nguyen :ntim]

We use the shared directory for the dev tools, and everything ha been going smooth so far. It also eases lots of work (edit 1 file instead of 3). I think we should go ahead with the shared directory.
Also I'm sure we don't have that much divergence across platforms with project chameleon.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Let's stick with the separate files for now, and discuss moving to a single shared file outside of this bug.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8432577 [details] [diff] [review]
moulting.patch

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

It's really hard to see what changed and what just got moved. Can you document what changed? It would have been better to do two patches, one that just moves the CSS and another that does the changes.

Since this is a pretty large change, we should make sure to land it at the beginning of a cycle so that it gets the most time to bake on Nightly for issues to be found and filed.

After you fix the following issues, please push to tryserver and request ui-review on the builds.

::: browser/base/content/aboutneterror/info.svg
@@ +1,3 @@
>  <svg xmlns="http://www.w3.org/2000/svg" version="1.1" x="0px" y="0px" viewBox="0 0 100 100" enable-background="new 0 0 100 100">
> +<g fill="#424E5A">
> +  <circle cx="50" cy="50" r="44" style="stroke: #424E5A; stroke-width: 11; fill: transparent;"/>

why did the colors of this change?

::: browser/themes/shared/in-content/common.css
@@ +15,5 @@
>    font: message-box;
>    font-size: 15px;
>    font-weight: normal;
>    margin: 0;
> +  color: #424E5A;

where does the changing of these colors come from? chameleon?

::: browser/themes/windows/jar.mn
@@ -161,5 @@
>  *       skin/classic/browser/preferences/preferences.css             (preferences/preferences.css)
>  *       skin/classic/browser/preferences/in-content/preferences.css  (preferences/in-content/preferences.css)
>          skin/classic/browser/preferences/in-content/favicon.ico      (../shared/incontentprefs/favicon.ico)
> -        skin/classic/browser/preferences/in-content/check.png        (../shared/incontentprefs/check.png)
> -        skin/classic/browser/preferences/in-content/[e-mail]     (../shared/incontentprefs/[e-mail])

These lines were deleted from the /skin/classic/browser/ section, but not from the /skin/classic/aero/browser/ section, causing the build to fail on Windows.

Do you have access to tryserver? You can use tryserver to see if the build passes on Windows.

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

vt, any update here?

Comment 22

[Valentin Tsatskin [:vt]]

Attachment: moulting-part-1.patch

Moves over generic in-content CSS styles from preferences.css to common.css

Comment 23

[Valentin Tsatskin [:vt]]

Attachment: moulting-part-2.patch

Moves over icon resources and their references to in-content folder

Comment 24

[Valentin Tsatskin [:vt]]

Attachment: moulting-part-3.patch

Merges styles created for network error pages and the ones brought in from part 1. This causes a visual change in neterror to match the look of about:preferences.

* info.svg's colours are updated to match the updated body colours.
* buttons in about:preferences now have cursor changes (carried over from netError)
  * enabled buttons get a pointer
  * disabled buttons get a not-allowed

Hopefully the cursor changes are a welcome change to the behaviour of these buttons. Michael, is this okay?

Comment 25

[Valentin Tsatskin [:vt]]

Looks like I lost my public key to push to try. Waiting on bug 1024353 to clear this up.

Comment 26

[Michael Maslaney [:mmaslaney] ([e-mail])]

Val, that works. Thanks.

Comment 27

[Valentin Tsatskin [:vt]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=966ea64b8985

Comment 28

[Tim Nguyen :ntim]

Here are some issues you might want to address :
- The netError page button doesn't use the same font as the inContent prefs buttons (at least on Windows). You might want to use font:message-box or font:inherit; to fix the issue.
- The Applications tab "table" should be higher (the patch made it less high)
- You might want to move the categories styling into common.css too, because we'll this later for the add-on manager and possibly for other pages in the UI.

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8439023 [details] [diff] [review]
moulting-part-1.patch

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

::: browser/themes/shared/in-content/common.css
@@ +162,5 @@
> +
> +/* xul buttons and menulists */
> +
> +button,
> +menulist {

This code was removed in part 3 but moved here in part 1. (not an issue, but just for clarity)

Comment 30

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Tim Nguyen [:ntim] from comment #28)
> - The Applications tab "table" should be higher (the patch made it less high)

Bug 1019157?

> - You might want to move the categories styling into common.css too, because
> we'll this later for the add-on manager and possibly for other pages in the
> UI.

Can we do this in a followup bug? Would be good to keep this bug's scope contained.

Comment 31

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8439023 [details] [diff] [review]
moulting-part-1.patch

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

::: browser/themes/linux/in-content/common.css
@@ +31,5 @@
> +  margin-bottom: 6px;
> +}
> +
> +checkbox {
> +  -moz-binding: url("chrome://global/content/bindings/checkbox.xml#checkbox");

Why are these bindings needed? They should be already applied via xul.css, which is automatically included for all XUL documents.

::: browser/themes/shared/in-content/common.css
@@ +2,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +%endif
> +@namespace html "http://www.w3.org/1999/xhtml";

Soooo..... namespaces are a PITA.

If you're in a HTML document, the default namespace will be HTML. Which means that in order to style XUL elements (yes, this can be required at times), you need to add the namespace to the CSS file.

And what we're wanting to do here is to include this file on *both* HTML and XUL doucments, which may be using a mix of HTML and XUL elements. Which makes dealing with namespaces in CSS rather awkward.

But, there is light at the end of the tunnel:
* Define a prefix for both HTML and XUL elements
* To style just HTML elements, use that prefix. Ditto for when we want to only style a XUL element.
* For styling an element that may be *either* XUL or HTML (eg, button) , use a wildcard prefix: *| (eg, *|button)

Which basically boils down to: You need to always use a namespace prefix (even if just a wildcard) to get the expected results, when mixing namespaces like we're trying to do here.

@@ +108,5 @@
> +  transition: background-color 50ms ease 0s;
> +}
> +
> +tab:hover {
> +  background-color: #ebebeb;

Consistancy: Using a mix of uppercase and lowercase hex digits in colors. Should all be the same.

Alas, mozilla-central isn't consistent whether it uses either uppercase or lowercase. But the webdev styleguide specifies lowercase, so my recommendation is to follow that.

Comment 32

[Richard Marti (:Paenglab)]

(In reply to Blair McBride [:Unfocused] from comment #31)
> Comment on attachment 8439023 [details] [diff] [review]
> moulting-part-1.patch
> 
> Review of attachment 8439023 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/linux/in-content/common.css
> @@ +31,5 @@
> > +  margin-bottom: 6px;
> > +}
> > +
> > +checkbox {
> > +  -moz-binding: url("chrome://global/content/bindings/checkbox.xml#checkbox");
> 
> Why are these bindings needed? They should be already applied via xul.css,
> which is automatically included for all XUL documents.

I have now no Linux box at hand, but if I remember correctly it's to revert to the default checkbox.css binding which is changed here: http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/global.css#16

Comment 33

[Valentin Tsatskin [:vt]]

Attachment: moulting-part-4.patch

* Made hex codes all lowercase and use short-hand where applicable
* Made buttons inherit their font to address Tim's comments

Comment 34

[Valentin Tsatskin [:vt]]

Attachment: moulting-part-5.patch

Added namespace for xul elements addressing Blair's comments

Comment 35

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8445566 [details] [diff] [review]
moulting-part-5.patch

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

::: browser/themes/shared/in-content/common.css
@@ +383,5 @@
>    color: #0095dd;
>  }
>  
> +xul|*.text-link:hover,
> +xul|*.inline-link:hover {

Missing html|a.inline-link here and for the :active rule below?

Comment 36

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8439023 [details] [diff] [review]
moulting-part-1.patch

r+ now, as attachment 8445566 [details] [diff] [review] resolves the issue described in comment 31.

Comment 37

[Tim Nguyen :ntim]

Comment on attachment 8439023 [details] [diff] [review]
moulting-part-1.patch

Couldn't apply patch :/ Can you rebase all your patches then fold them into one ?

Comment 38

[Blair McBride [:Unfocused] (UNAVAILABLE)]

See comment 37.

Comment 39

[Richard Marti (:Paenglab)]

Valentin, are you still working on this bug?

Locally I have updated patches and could them upload and you could check if everything is okay. There there a lot of changes in in-content prefs which had to update in your patches like for the in-content dialogs and the new styling.

Comment 40

[Richard Marti (:Paenglab)]

Attachment: moulting-part-1.patch

Updated patch. I set r+ but I'm asking Blair if he wants to review again.

Comment 41

[Richard Marti (:Paenglab)]

Attachment: moulting-part-2.patch

Updated patch.

Comment 42

[Richard Marti (:Paenglab)]

Attachment: moulting-part-3.patch

Updated patch.

Comment 43

[Richard Marti (:Paenglab)]

Attachment: moulting-part-4.patch

Updated patch.

Comment 44

[Richard Marti (:Paenglab)]

Attachment: moulting-part-5.patch

Updated patch.

Blair, do you want to re-review this patches or is it okay to check-in? There where numerous changes because of my changes for the in-content prefs.

Comment 45

[Richard Marti (:Paenglab)]

Attachment: moulting-part-5.patch

Found a small bug.

Comment 46

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Yea, I'll re-review. Thanks!

Comment 47

[Richard Marti (:Paenglab)]

I made a Try run with this patches with tests on Linux and OS X where I haven't tested: https://tbpl.mozilla.org/?tree=Try&rev=a57698c89522

Comment 48

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8467177 [details] [diff] [review]
moulting-part-1.patch

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

::: browser/themes/linux/in-content/common.css
@@ +1,5 @@
> +/* - This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +   - You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +%include ../../shared/in-content/common.css

The file in /shared/ should be renamed to common.inc.css if it's being included like this, to make it obvious how it's intended to be used.

Comment 49

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8467179 [details] [diff] [review]
moulting-part-3.patch

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

r+ with the following fix:

::: browser/themes/shared/in-content/common.css
@@ +155,5 @@
>  button:not([disabled="true"]):hover,
>  colorpicker[type="button"]:not([disabled="true"]):hover,
>  menulist:not([disabled="true"]):hover {
>    background-color: #EBEBEB;
> +  cursor: pointer;

We don't want this here anymore - it was removed by bug 1031571.

Comment 50

[Richard Marti (:Paenglab)]

Attachment: moulting-part-1.patch

Changed to common.inc.css. This needs changes in all patches.

Comment 51

[Richard Marti (:Paenglab)]

Attachment: moulting-part-2.patch

Updated

Comment 52

[Richard Marti (:Paenglab)]

Attachment: moulting-part-3.patch

Removed the cursor: pointer;

Comment 53

[Richard Marti (:Paenglab)]

Attachment: moulting-part-4.patch

Updated

Comment 54

[Richard Marti (:Paenglab)]

Attachment: moulting-part-5.patch

Updated

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/1890f8697f84
https://hg.mozilla.org/integration/fx-team/rev/0704d49fcc7a
https://hg.mozilla.org/integration/fx-team/rev/0b66cb27bae3
https://hg.mozilla.org/integration/fx-team/rev/0138ce9c6040
https://hg.mozilla.org/integration/fx-team/rev/b771af2fac62

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1890f8697f84
https://hg.mozilla.org/mozilla-central/rev/0704d49fcc7a
https://hg.mozilla.org/mozilla-central/rev/0b66cb27bae3
https://hg.mozilla.org/mozilla-central/rev/0138ce9c6040
https://hg.mozilla.org/mozilla-central/rev/b771af2fac62

Comment 57

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Woo! Thanks for finishing this off, Richard :)

Comment 58

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Oh, in terms of QA: Any potential regression will manifest as a visual regression in about:preferences

Comment 59

[Camelia Badau [:cbadau], Desktop QA]

Verified on Windows 7 64bit, Windows 8 32bit, Ubuntu 13.10 32bit and Mac OSX 10.7.5 using latest Nightly 34.0a1 (buildID: 20140811030203) and the following mentions should be done: 
- under Applications, the arrows are not shown anymore when sorting the two columns
- under Advanced - Network : the "Remove.." button from the "Offline Web Content and User Data" section is disabled; if you select the website which appears on the "The following websites are allowed to store data for offline use:", the "Remove.." button become enabled. The problem here is that this website remains selected and the "Remove.." button also remains enabled. 
Are those issues related to this bug or should I file news ones?

Comment 60

[Richard Marti (:Paenglab)]

(In reply to Camelia Badau, QA [:cbadau] from comment #59)
> - under Applications, the arrows are not shown anymore when sorting the two
> columns

This is a issue from this bug. Please file a new bug and cc me to this.

> - under Advanced - Network : the "Remove.." button from the "Offline Web
> Content and User Data" section is disabled; if you select the website which
> appears on the "The following websites are allowed to store data for offline
> use:", the "Remove.." button become enabled. The problem here is that this
> website remains selected and the "Remove.." button also remains enabled. 
> Are those issues related to this bug or should I file news ones?

This shouldn't be a issue from this bug. You could check this in the old prefs by changing the pref 'browser.preferences.inContent' and testing it again. I never had any entry in this list to test.

Comment 61

[Tim Nguyen :ntim]

Also, in the neterror page, the try again button is thinner than before

Comment 62

[Richard Marti (:Paenglab)]

Yes, in netError.css there was a button {min-width: 150px} rule which is now missing.

Blair, should I add this rule directly to #errorTryAgain in netError.css as it's the only button there or to html|button in common.css?

Comment 63

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Richard Marti (:Paenglab) from comment #62)
> Blair, should I add this rule directly to #errorTryAgain in netError.css as
> it's the only button there

I'd go with this - that button is a bit special, as we purposefully want it larger than normal to make it super easy to hit quickly.

And lets file new bugs for any such regressions. This bug is done and closed.

Comment 64

[Camelia Badau [:cbadau], Desktop QA]

(In reply to Richard Marti (:Paenglab) from comment #60)
> (In reply to Camelia Badau, QA [:cbadau] from comment #59)
> > - under Applications, the arrows are not shown anymore when sorting the two
> > columns
> 
> This is a issue from this bug. Please file a new bug and cc me to this.

I filled bug 1052315. 

> 
> > - under Advanced - Network : the "Remove.." button from the "Offline Web
> > Content and User Data" section is disabled; if you select the website which
> > appears on the "The following websites are allowed to store data for offline
> > use:", the "Remove.." button become enabled. The problem here is that this
> > website remains selected and the "Remove.." button also remains enabled. 
> > Are those issues related to this bug or should I file news ones?
> 
> This shouldn't be a issue from this bug. You could check this in the old
> prefs by changing the pref 'browser.preferences.inContent' and testing it
> again. I never had any entry in this list to test.

I verified this in old prefs (when the "Options" opens in a window, not in content) and there is the same behaviour: the website and the "Remove.." button remain enabled until the window is closed and then reopened (for in-content preferences, the website and the "Remove.." button remain enabled remain enabled until reloading the tab). Do you think that I should file a bug?

Comment 65

[Richard Marti (:Paenglab)]

Yes, please file a bug.

Comment 66

[Tim Nguyen :ntim]

(In reply to Camelia Badau, QA [:cbadau] from comment #64)
> > > - under Advanced - Network : the "Remove.." button from the "Offline Web
> > > Content and User Data" section is disabled; if you select the website which
> > > appears on the "The following websites are allowed to store data for offline
> > > use:", the "Remove.." button become enabled. The problem here is that this
> > > website remains selected and the "Remove.." button also remains enabled. 
> > > Are those issues related to this bug or should I file news ones?
> > 
> > This shouldn't be a issue from this bug. You could check this in the old
> > prefs by changing the pref 'browser.preferences.inContent' and testing it
> > again. I never had any entry in this list to test.
> 
> I verified this in old prefs (when the "Options" opens in a window, not in
> content) and there is the same behaviour: the website and the "Remove.."
> button remain enabled until the window is closed and then reopened (for
> in-content preferences, the website and the "Remove.." button remain enabled
> remain enabled until reloading the tab). Do you think that I should file a
> bug?
I don't think it's an actual bug, as long as the Remove button still works.