1294840 - 'add-filter' , 'toggle-preset' and 'add-preset (plus button)' are not vertically aligned

Status: VERIFIED FIXED

(DevTools :: Inspector: Rules, defect, P2)

Description

[[:Towkir] Ahmed]

Attachment: vertical_alignment_mockup.jpg

the plus button (add-filter) and the toggle button (toggle-preset) are not vertically aligned with the 'Select a Filter' dropdown.

See attached screenshot to understand the issue.

Comment 1

[[:Towkir] Ahmed]

Attachment: vertical_alignment.patch

Hey, Julian.
What is your opinion about this ?

Comment 2

[Julian Descottes [:jdescottes]]

Comment on attachment 8780687 [details] [diff] [review]
vertical_alignment.patch

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

Here the display is quite different between Windows, OSX, and Linux. Mostly because the default font-size is much bigger on Linux. 

Adding a margin top will fix the issue for Linux, but will change the layout on other platforms (will actually increase the select's height). I would advise to add "align-items: center" to the .footer selector here.

::: devtools/client/shared/widgets/filter-widget.css
@@ +232,4 @@
>    font-size: 0;
>    vertical-align: middle;
>    cursor: pointer;
> +  margin: 5px 5px;

FYI: this is equivalent to margin: 5px;

Comment 3

[[:Towkir] Ahmed]

Attachment: vertical_alignment.patch

Okay then, Here it is ...

Comment 4

[[:Towkir] Ahmed]

Attachment: side-effect.JPG

Oh, wait
there is another issue which rises with the fix of this bug, the "plus / add" button beside the "Preset Name" gets a little bit lower. (see attachment)
this happens because of the margin on : https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/widgets/filter-widget.css#46 
removing the margin (which means removing the rule on line 45) fixes this too. 
What do you think ? 
Should it be done ? If yes, then should there be another bug ? or I can update the current patch ?

Comment 5

[Julian Descottes [:jdescottes]]

(In reply to [:Towkir] Ahmed from comment #4)
> Created attachment 8780924 [details]
> side-effect.JPG
> 
> Oh, wait
> there is another issue which rises with the fix of this bug, the "plus /
> add" button beside the "Preset Name" gets a little bit lower. (see
> attachment)
> this happens because of the margin on :
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/
> widgets/filter-widget.css#46 
> removing the margin (which means removing the rule on line 45) fixes this
> too. 
> What do you think ? 
> Should it be done ? If yes, then should there be another bug ? or I can
> update the current patch ?

Yes good catch, please remove the margin-top: 4px here. You can amend your changeset and resubmit it for review, no need for a separate bug here. Thanks!

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8780853 [details] [diff] [review]
vertical_alignment.patch

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

Looks good, as discussed above let's also remove the margin-top rule.

Comment 7

[[:Towkir] Ahmed]

As the patch will be updated, updating the summary for now, so that it becomes easy to understand the exact purpose and fix of this bug.

Comment 8

[[:Towkir] Ahmed]

Attachment: vertical_alignment.patch

Hope this one fixes all of the mentioned issues

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8781208 [details] [diff] [review]
vertical_alignment.patch

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

Thanks for the patch! I'm giving an r+ here, but you need to amend your patch and change the commit message.

First of all the summary is a bit too long here: 
"Bug 1294840 - Fixed vertical alignment of 'add-filter' , 'toggle-preset' and 'add-preset (plus button)' in devtools: css rule inspector" -> Maybe instead of 'add-filer' etc... just say 'filter widget buttons'. 

Also please add ';r=jdescottes' at the end of the summary.

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c60c749a1af7

Thanks!

Comment 10

[[:Towkir] Ahmed]

Attachment: vertical_alignment.patch

All done.

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8781585 [details] [diff] [review]
vertical_alignment.patch

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

Thanks! FYI, if you already have r+ on a patch, you can simply set the r+ flag on your new attachment yourself :) 
(unless of course you made huge modifications since the version that got r+, which would be worth reviewing again).

Comment 12

[Julian Descottes [:jdescottes]]

Try is green except for unrelated intermittents -> checkin-needed.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/e12e7f9d42fe
Fix vertical alignment of 'filter widget buttons' in devtools: css rule inspector. r=jdescottes

Comment 14

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

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

Comment 15

[Sayed [:LiquidO2]]

I have reproduced this bug with Nightly 51.0a1 (2016-08-13) on Elementary OS 64bit. 

This bug's fix is now verified on Latest Nightly 51.0a1

Build ID  	20160901030202
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0

[bugday-20160831]

Comment 16

[Md Maruf Rahman]

I have reproduced this bug with Nightly 51.0a1 (2016-08-12) on Windows 7, 64 Bit !

This bug's reproduce is verified with latest Nightly 

Build  ID  20160831030224
User Agent Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0
[bugday-20160831]

Comment 17

[[:Towkir] Ahmed]

As per comment 15 and comment 16