848134 - condense metro css attributes whose only value is true(Mbrubeck Memorial Bug)

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Theme, defect)

Description

[Allison Naaktgeboren :ally]

As suggested at the end https://bugzilla.mozilla.org/show_bug.cgi?id=794028#c23, there is a more concise way to write flags. In changing mine, I noticed that we have a slew of like code in platform.css, which led me to see that we have a slew elsehwere as well. So, I went & cleaned some of it up.

Attributes I noticed that are only set to true in the metro css:
-disabled
-selected
-forwarddisabled
-checked
-panning
-showing
-firstshow
-paused
-muted
-customColorPresent
-customImagePresent
-tabsonly

Attributes that looked like candidates, but turned out not to be:
-shownfrom
-collapsed(neterror.css uses t/f value of this one)



I dub this the mbrubeck memorial bug.

Comment 1

[Allison Naaktgeboren :ally]

Attachment: draft 0 part 1/1

Since this covers a wide range of ui impacting css, I nominate this for qa smoke testing/beating

Comment 2

[Matt Brubeck (:mbrubeck)]

Comment on attachment 721468 [details] [diff] [review]
draft 0 part 1/1

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

Thanks!  :)

::: browser/metro/theme/browser.css
@@ +351,5 @@
>    min-height: 48px !important;
>    max-height: 48px !important;
>  }
>  
> +#back-button[disabled] {

It looks like code in BrowserUI._updateButtons sets disabled="false" instead of removing the attribute, so we will need to either change that code or leave this selector alone.  (Ideally we should change the code, since a lot of the toolkit styles assumse that "disabled" can only be set to "true".)

@@ +372,5 @@
>  #forward-button image {
>    margin: -1px 0 1px 0 !important;
>  }
>  
> +#unified-back-forward-button > #forward-button[disabled] {

...and this one too.

::: browser/metro/theme/content.css
@@ +275,5 @@
>  
>  input[type="button"][disabled],
>  input[type="submit"][disabled],
>  input[type="reset"][disabled],
> +button[disabled] {

MasterPasswordUI sets disabled="false" -- we could fix that to use removeAttribute.

::: browser/metro/theme/platform.css
@@ +292,5 @@
>    color: black;
>    background-color: white;
>  }
>  
> +richlistitem:hover:active:not([selected]):not([nohighlight="true"]) {

"nohighlight" was used in XUL Fennec but it's not used in Metro.  You can just remove the :not([nohighlight="true"]) from this selector completely.

::: browser/metro/theme/touchcontrols.css
@@ +39,5 @@
>    transform: translateX(21px);
>    background: url("chrome://browser/skin/images/pause-hdpi.png") no-repeat center;
>  }
>  
> +.playButton[paused] {

Unfortunately, there's some toolkit code that sets paused="false" so we can't take this change.  We should probably leave all the selectors in this file alone.  Fortunately I think we can eventually remove this stylesheet and just use the default toolkit one (see bug 704229 for some context).

Comment 3

[Allison Naaktgeboren :ally]

Attachment: parft 1 part 1

'selected' suffered from the same problem as disabled, but I think got/fixed all of that.

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 723017 [details] [diff] [review]
parft 1 part 1

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

Awesome, thanks again.  Feel free to check in with r=mbrubeck with the minor points below addressed:

::: browser/metro/base/content/bindings/grid.xml
@@ -31,5 @@
>              // 'selection' and 'selected' are confusingly overloaded here
>              // as richgrid is adopting multi-select behavior, but select/selected are already being
>              // used to describe triggering the default action of a tile
>              if (this._selectedItem){
> -              this._selectedItem.selected = false;

I think we should either fix the richgriditem "selected" setter to use removeAttribute (and continue using it here), or remove the setter (making it a read-only attribute).

::: browser/metro/base/content/browser-ui.js
@@ +591,5 @@
>      let browser = Browser.selectedBrowser;
> +    if (browser.canGoBack) {
> +      this._back.removeAttribute("disabled");
> +    }
> +    else {

Style nit: Metro front-end code cuddles the else between the braces, all on one line.

No particular rationale besides consistency (and obedience to the unwritten style guide as handed down by mfinkle).

Comment 5

[Allison Naaktgeboren :ally]

Attachment: draft 2 part 1/1

You are way more confident in my abilities than I am. :)

Sending back for review, mainly to ensure I got the right setter in the intended manner.  At least it should be a really quick review

Comment 6

[Allison Naaktgeboren :ally]

https://hg.mozilla.org/integration/mozilla-inbound/rev/545649dc10e7

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/545649dc10e7