980339 - about:addons squaring

Status: VERIFIED FIXED

(Toolkit :: Themes, defect)

Description

[Peter Henkel [:Terepin]]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140306030201

Steps to reproduce:

Add-ons list and section selector needs to be squared and not rounded.

Comment 1

[Justin Dolske [:Dolske]]

Not tracking for Australis, but we'd of course take your patch. :)

Comment 2

[Tim Nguyen :ntim]

Gonna give a try at this. I'm going to submit a pull request on Github soon.

Comment 3

[Tim Nguyen :ntim]

Attachment: bug_980339.patch

This patch is just for Windows 8, since I see no point of squaring borders for Windows Aero.

Comment 4

[Mike de Boer [:mikedeboer]]

Comment on attachment 8395276 [details] [diff] [review]
bug_980339.patch

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

Looking good, except for the media queries you used.

::: toolkit/themes/windows/global/inContentUI.css
@@ +73,5 @@
>  %ifdef WINDOWS_AERO
>  @media (-moz-windows-glass) {
> +  *|*.main-content {
> +    border-radius: 5px;
> +  }

I think (-moz-windows-glass) is too restrictive.

The pattern we use to enable styles on other vista and 7 is

```css
%ifdef WINDOWS_AERO
@media (-moz-os-version: windows-vista),
       (-moz-os-version: windows-win7) {
  /* do stuff... */
}
%endif
```

Please use this one instead.

::: toolkit/themes/windows/mozapps/extensions/extensions.css
@@ +194,1 @@
>  }

Same here.

Comment 5

[Tim Nguyen :ntim]

Attachment: Patch v2

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8397266 [details] [diff] [review]
Patch v2

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

Please see my comment in bug 980418.

Comment 7

[Tim Nguyen :ntim]

Attachment: Patch v3

Comment 8

[Tim Nguyen :ntim]

Comment on attachment 8397936 [details] [diff] [review]
Patch v3

Forgot to change something

Comment 9

[Tim Nguyen :ntim]

Attachment: Patch v4

Comment 10

[Mike de Boer [:mikedeboer]]

Comment on attachment 8397938 [details] [diff] [review]
Patch v4

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

r=me with my last comment fixed (please upload a new patch for that.)

Thanks!

::: toolkit/themes/windows/global/inContentUI.css
@@ +78,5 @@
> +    border-radius: 5px;
> +  }
> +%ifdef WINDOWS_AERO
> +}
> +%endif

You may remove the

```
%endif

%ifdef WINDOWS_AERO
```

These preprocessor blocks are like a markup language of sorts; you opened a WINDOWS_AERO section before, so you don't need to close it and open it again right after closing it.

Comment 11

[Tim Nguyen :ntim]

Attachment: Patch v5

Yup, didn't notice it was duplicated.

Btw, do you know how to make a "shortcut" in Mozilla build ? I'd like to be able to open the files with sublime text directly in mozilla build (instead of using gvim). For now, I have to navigate to the file directory then open the file with sublime text which isn't convenient.

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8398578 [details] [diff] [review]
Patch v5

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

Following official policy, requesting s-r from a toolkit peer.

::: toolkit/themes/windows/global/inContentUI.css
@@ +77,5 @@
> +  *|*.main-content {
> +    border-radius: 5px;
> +  }
> +%ifdef WINDOWS_AERO
> +}

nit: please keep the empty newline here.

Comment 13

[Tim Nguyen :ntim]

Attachment: Patch v6. r=mdeboer; s-r=bmcbride

Same as Patch v5, but with new line break.

Comment 14

[Mike de Boer [:mikedeboer]]

Pushed as: https://hg.mozilla.org/integration/fx-team/rev/b53faee9792e

Comment 15

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #12)
> Comment on attachment 8398578 [details] [diff] [review]
> Patch v5
> 
> Review of attachment 8398578 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Following official policy, requesting s-r from a toolkit peer.

Official policy is to get review from a peer. super-review is something different: http://www.mozilla.org/hacking/reviewers.html

Comment 16

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #15)
> Official policy is to get review from a peer. super-review is something
> different: http://www.mozilla.org/hacking/reviewers.html

Duly noted, will remember.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 18

[Tim Nguyen :ntim]

Comment on attachment 8400123 [details] [diff] [review]
Patch v6. r=mdeboer; s-r=bmcbride

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: Awkward rounded corners in add-on manager for Win 8
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): Low, CSS only
String or IDL/UUID changes made by this patch: None

Comment 19

[Mike de Boer [:mikedeboer]]

Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/f0c069af1f3a
Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/cb7f81834560

Comment 20

[Cornel Ionce [:noni] [Hubs QA]]

Verified issue fixed on Windows 8.1 (32bit and 64bit architectures) using:
- latest Firefox Nightly (build ID: 20140407030203)
- latest Firefox Aurora (build ID: 20140407004002)
- Firefox 29 beta 6 (build ID: 20140407135746).

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Backed out on beta for causing bug 999080:
https://hg.mozilla.org/releases/mozilla-beta/rev/02556a393ed8

Comment 22

[Cornel Ionce [:noni] [Hubs QA]]

Verified disabled on Fx 29 RC, build ID: 20140421221237.