Status

()

defect
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: Terepin, Assigned: ntim)

Tracking

Trunk
mozilla31
All
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 verified disabled, firefox30 verified, firefox31 verified)

Details

(Whiteboard: [Australis:P-])

Attachments

(1 attachment, 5 obsolete attachments)

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.
Blocks: 978003
Component: Untriaged → Theme
Hardware: x86_64 → All
Version: 30 Branch → Trunk
Component: Theme → Themes
Product: Firefox → Toolkit
Not tracking for Australis, but we'd of course take your patch. :)
Whiteboard: [Australis:P-]
No longer blocks: 978003
Depends on: 978003
Blocks: theme-win8
Gonna give a try at this. I'm going to submit a pull request on Github soon.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Posted patch bug_980339.patch (obsolete) — Splinter Review
This patch is just for Windows 8, since I see no point of squaring borders for Windows Aero.
Attachment #8395276 - Flags: review?(mdeboer)
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Blocks: 980418
Blocks: 980413
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.
Attachment #8395276 - Flags: review?(mdeboer) → feedback+
Posted patch Patch v2 (obsolete) — Splinter Review
Attachment #8395276 - Attachment is obsolete: true
Attachment #8397266 - Flags: review?(mdeboer)
Comment on attachment 8397266 [details] [diff] [review]
Patch v2

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

Please see my comment in bug 980418.
Attachment #8397266 - Flags: review?(mdeboer)
Posted patch Patch v3 (obsolete) — Splinter Review
Attachment #8397266 - Attachment is obsolete: true
Attachment #8397936 - Flags: review?(mdeboer)
Comment on attachment 8397936 [details] [diff] [review]
Patch v3

Forgot to change something
Attachment #8397936 - Flags: review?(mdeboer)
Posted patch Patch v4 (obsolete) — Splinter Review
Attachment #8397938 - Flags: review?(mdeboer)
Attachment #8397936 - Attachment is obsolete: true
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.
Attachment #8397938 - Flags: review?(mdeboer) → review+
Posted patch Patch v5 (obsolete) — Splinter Review
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.
Attachment #8397938 - Attachment is obsolete: true
Attachment #8398578 - Flags: review?(mdeboer)
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.
Attachment #8398578 - Flags: superreview?(bmcbride)
Attachment #8398578 - Flags: review?(mdeboer)
Attachment #8398578 - Flags: review+
Attachment #8398578 - Flags: superreview?(bmcbride) → superreview+
Same as Patch v5, but with new line break.
Attachment #8398578 - Attachment is obsolete: true
Attachment #8400123 - Flags: superreview+
Attachment #8400123 - Flags: review+
Keywords: checkin-needed
Pushed as: https://hg.mozilla.org/integration/fx-team/rev/b53faee9792e
Keywords: checkin-needed
Whiteboard: [Australis:P-] → [Australis:P-][fixed-in-fx-team]
(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
(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.
https://hg.mozilla.org/mozilla-central/rev/b53faee9792e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P-][fixed-in-fx-team] → [Australis:P-]
Target Milestone: --- → mozilla31
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
Attachment #8400123 - Flags: approval-mozilla-beta?
Attachment #8400123 - Flags: approval-mozilla-aurora?
Attachment #8400123 - Flags: approval-mozilla-beta?
Attachment #8400123 - Flags: approval-mozilla-beta+
Attachment #8400123 - Flags: approval-mozilla-aurora?
Attachment #8400123 - Flags: approval-mozilla-aurora+
Keywords: checkin-needed
Flags: needinfo?(mdeboer)
Keywords: checkin-needed
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).
Depends on: 999080
Verified disabled on Fx 29 RC, build ID: 20140421221237.
You need to log in before you can comment on or make changes to this bug.