Note: There are a few cases of duplicates in user autocompletion which are being worked on.

CSS - Circles in ruleview (.ruleview-swatch) are misplaced by 0.1em

VERIFIED FIXED in Firefox 54

Status

()

Firefox
Developer Tools: CSS Rules Inspector
P3
normal
VERIFIED FIXED
7 months ago
2 months ago

People

(Reporter: arni2033, Assigned: Towkir, Mentored)

Tracking

({good-first-bug})

Trunk
Firefox 54
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox54 verified)

Details

(Whiteboard: [lang=css])

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

7 months ago
>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR_1:
1. Open url [1]
2. Open devtools -> inspector -> rules, inspect <body> element
3. Look at the swatches in ruleview (increase zoom level in devtools to see better)

AR:  Swatches are noticeably misplaced, on HiDPI (125%) it's noticeable even w/o changing zoom level
ER:  Swatches should be placed correctly

Note:
 This happens because width and height of .ruleview-swatch is defined as "0.9em",
 while background-size of all swatches is defined as "1em auto".

> [1] data:text/html,<style>body{filter:none;color:transparent;animation:ease;transform:rotate(0deg)}
(Reporter)

Updated

7 months ago
No longer blocks: 1277113
(Reporter)

Updated

7 months ago
Component: Untriaged → Developer Tools
Component: Developer Tools → Developer Tools: CSS Rules Inspector
Created attachment 8824430 [details]
Capture.PNG

What do you mean by misplaced? I've attached a screenshot of what it looks like for me (tested on 51 and 53, they look the same in both). It would help if you could also attach a screenshot.
Marking as WORKSFORME for now. Feel free to re-open with a screenshot.
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → WORKSFORME

Comment 3

7 months ago
Your screenshot, that was taken on HiDPI, perfectly describes the bug.
Icons in all circles are misaligned (not placed in the center).

Bug 1327981 is related, please add it in "See also" list.
Flags: needinfo?(pbrosset)
Ok thanks for confirming. So the problem is the icons inside the circle that are slightly offset to the right.
Status: RESOLVED → REOPENED
Flags: needinfo?(pbrosset)
Priority: -- → P3
Resolution: WORKSFORME → ---

Comment 5

6 months ago
https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/rules.css#369-370

Change this to 1em
Keywords: good-first-bug
Whiteboard: [lang=css]

Updated

6 months ago
Status: REOPENED → NEW
(Assignee)

Comment 6

6 months ago
Hi Tim,
According to comment 5 making you a mentor here.
Will submit a patch soon, hope you will take care about that :)
Assignee: nobody → 3ugzilla
Mentor: ntim.bugs@gmail.com
Status: NEW → ASSIGNED
(Assignee)

Comment 7

6 months ago
Created attachment 8825117 [details] [diff] [review]
ruleview-swatch.patch

Looks like Tim is not reviewing for now.
Hi Patrick, can you have a look at this ?
Thanks
Attachment #8825117 - Flags: review?(pbrosset)
Comment on attachment 8825117 [details] [diff] [review]
ruleview-swatch.patch

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

Tested on Windows 10. It does indeed seem to address the problem nicely. I wonder why it was 0.9em to begin with.
Tim: any idea about the history for this thing? Are we likely to re-surface another problem by bumping this to 1em?
In any case, simple enough CSS change that can get a quick R+. But let's wait for Tim's feedback on this to land it.
Attachment #8825117 - Flags: review?(pbrosset) → review+

Comment 9

6 months ago
(In reply to Patrick Brosset <:pbro> from comment #8)
> Comment on attachment 8825117 [details] [diff] [review]
> ruleview-swatch.patch
> 
> Review of attachment 8825117 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Tested on Windows 10. It does indeed seem to address the problem nicely. I
> wonder why it was 0.9em to begin with.
> Tim: any idea about the history for this thing? Are we likely to re-surface
> another problem by bumping this to 1em?
> In any case, simple enough CSS change that can get a quick R+. But let's
> wait for Tim's feedback on this to land it.

Looking at hg blame, the dimensions were changed from 1em to .9em by bug 1154059, but on Windows, it seems like bug 1154059 is more visible with .9em than with 1em.

As for OSX, I've noticed no change.

I don't know about linux though.

If bug 1154059 occurs again on linux, I think using border instead of box-shadow for the border would be a better fix.

Updated

6 months ago
Attachment #8825117 - Flags: review+
(Assignee)

Comment 10

6 months ago
Looks like the patch is good to go for now. Is it Tim ?
Flags: needinfo?(ntim.bugs)

Comment 11

6 months ago
(In reply to [:Towkir] Ahmed from comment #10)
> Looks like the patch is good to go for now. Is it Tim ?

Have you tested if bug 1154059 occurs on Linux with the patch applied ? If it doesn't occur, feel free to mark as checkin-needed.
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 12

6 months ago
(In reply to Tim Nguyen :ntim from comment #11)
> Have you tested if bug 1154059 occurs on Linux with the patch applied ? If
> it doesn't occur, feel free to mark as checkin-needed.
Sorry, I don't have a HiDPI monitor and which I have is not that good, So I can't be sure about that and I don't want to be in doubt about this.

Can we get help from the qa team or anyone else (you know) here ?
Thanks
Flags: needinfo?(ntim.bugs)

Comment 13

6 months ago
(In reply to [:Towkir] Ahmed from comment #12)
> (In reply to Tim Nguyen :ntim from comment #11)
> > Have you tested if bug 1154059 occurs on Linux with the patch applied ? If
> > it doesn't occur, feel free to mark as checkin-needed.
> Sorry, I don't have a HiDPI monitor and which I have is not that good, So I
> can't be sure about that and I don't want to be in doubt about this.
It doesn't seem HiDPI specific (the screenshot in that bug is normal DPI).

Just check if you can reproduce on your machine, and it should be enough really. Someone will file a bug if not.
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 14

6 months ago
Created attachment 8832048 [details]
rulevew-swatch-light.gif

I did test this on linux, looks good. I am attaching screencast for both light and dark theme, 
Before landing this, can we remove that negative 1px margin-top ??

See what changes when I toggle the margin.
Let me know what you guys think
(Assignee)

Comment 15

6 months ago
Created attachment 8832050 [details]
rulevew-swatch-dark.gif

Here is the result on dark theme.
Flags: needinfo?(pbrosset)
Flags: needinfo?(ntim.bugs)

Comment 16

6 months ago
(In reply to [:Towkir] Ahmed from comment #14)
> Created attachment 8832048 [details]
> rulevew-swatch-light.gif
> 
> I did test this on linux, looks good. I am attaching screencast for both
> light and dark theme, 
> Before landing this, can we remove that negative 1px margin-top ??
> See what changes when I toggle the margin.
Looks good with the margin change.
Flags: needinfo?(pbrosset)
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 17

6 months ago
Created attachment 8832324 [details] [diff] [review]
ruleview-swatch.patch

We need to see the results from OSX too.
Attachment #8825117 - Attachment is obsolete: true
Attachment #8832324 - Flags: review?(pbrosset)
Comment on attachment 8832324 [details] [diff] [review]
ruleview-swatch.patch

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

Thanks!
See my comment below.
I'll R+ this now. Feel free to attach a new patch and mark it as R+ yourself.

::: devtools/client/themes/rules.css
@@ -371,3 @@
>    vertical-align: middle;
> -  /* align the swatch with its value */
> -  margin-top: -1px;

I tested this on Windows and Mac. It looks fine on Windows, but on Mac, the swatches are a little bit too low I think. I would like to keep this negative margin-top.
Attachment #8832324 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 19

6 months ago
Created attachment 8833272 [details] [diff] [review]
ruleview-swatch.patch

Alright, here is it. I did r+ myself.
Attachment #8832324 - Attachment is obsolete: true
Attachment #8833272 - Flags: review+
(Assignee)

Comment 20

6 months ago
I hope it is good to go now
Keywords: checkin-needed

Comment 21

6 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a9f30b32cfc
CSS - Circles in ruleview (.ruleview-swatch) are now placed properly. r=pbro
Keywords: checkin-needed

Comment 22

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5a9f30b32cfc
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago6 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Screenshots of the change: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=94079d43835ff229dce0f9b226cedd64c6aaef60&newProject=mozilla-central&newRev=09b28b955a478f5a63b96db165a7974b367e14c3&filter=%5E%28win%7Cosx%29.*devtools

It seems like it made the swatches larger, not only re-positioning them. No reply necessary if it's fine now.

Comment 24

2 months ago
I have reproduced this bug with Nightly 53.0a1 (2017-01-01) on Windows 10, 64 bit!

The bug's fix is now verified on Latest Beta 54.0b7

Build ID 	20170511130324
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0

[testday-20170512]

Updated

2 months ago
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
You need to log in before you can comment on or make changes to this bug.