1464939 - Split console toolbar layout changes when close button is focussed

Status: RESOLVED FIXED

(DevTools :: Console, defect, P2)

Description

[Brian Birtles (:birtles)]

STR:
1. Open and focus the DevTools on any panel other than the console.
2. Press Esc to open the split console.
3. Focus on the "Filter output" text box
4. Press Tab twice to focus the "X" close button

Expected results:
The button is highlighted but otherwise the layout doesn't change.

Actual results:
The "X" and the "Persist Logs" control shift to the right by about 1px

You can see this even if you just close the panel by pressing the "X" button since while you have the mouse down the X is focused.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Mantaroh, could you have a look ?

Comment 2

[Mantaroh Yoshinaga[:mantaroh]]

Thanks Nicolas,

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #1)
> Mantaroh, could you have a look ?

Yes! I'll look into this.

Comment 3

[Mantaroh Yoshinaga[:mantaroh]]

We need to set the margin and padding to zero. Furthermore, we should set 'height:100%' to this button.

WIP: https://treeherder.mozilla.org/#/jobs?repo=try&revision=82247665c0681aeb75c61c2c0980f27a677e915b (This is only build which confirming each platform for myself.)

Comment 4

[Mantaroh Yoshinaga[:mantaroh]]

We need to remove right margin in the case of platform is not macOS. (same to bug 1463355) The second patch of this bug will address it.

Comment 5

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1464939 - Part 1.Remove padding-inline-start and padding-inline-end style from the focusring of devtools's button and tab.

Review commit: https://reviewboard.mozilla.org/r/248814/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248814/

Comment 6

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1464939 - Part 2.Remove right margin in case of the platform is not 'mac'.

Review commit: https://reviewboard.mozilla.org/r/248816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248816/

Comment 7

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8982966 [details]
Bug 1464939 - Part 1.Remove padding-inline-start and padding-inline-end style from the focusring of devtools's button and tab.

Sorry, I cleared the r? flag since I want to fix by using the another style.

I think this bug might be related to bug 1444793. I added the moz-focusring style which set the 'padding-inline-end' and 'padding-inline-start' to zero in bug 1444793. This modification affected devtool's buttons which has a padding style. [1]

I think that we didn't need to remove the padding-inline-* from buttons since devtools-button has a padding style[2].

[1] https://hg.mozilla.org/mozilla-central/rev/8b4050603e3d#l1.17
[2] https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/devtools/client/themes/common.css#286

Comment 8

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8982966 [details]
Bug 1464939 - Part 1.Remove padding-inline-start and padding-inline-end style from the focusring of devtools's button and tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248814/diff/1-2/

Comment 9

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8982967 [details]
Bug 1464939 - Part 2.Remove right margin in case of the platform is not 'mac'.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248816/diff/1-2/

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Screenshot at Jun 07 13-13-25.png

Thanks for the patch Mantaroh. 
I don't see the flicker anymore with your patch. 
However I noticed the close button overlaps a bit with the messages below it.
There's seem to be a margin-top on the button which would explain this (the button now being 100%) ?

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8982966 [details]
Bug 1464939 - Part 1.Remove padding-inline-start and padding-inline-end style from the focusring of devtools's button and tab.

https://reviewboard.mozilla.org/r/248814/#review256334

::: devtools/client/themes/common.css
(Diff revision 2)
> -  padding-inline-end: 0;
> -  padding-inline-start: 0;

it would be interesting to know why it was set in the first place.

::: devtools/client/themes/webconsole.css:606
(Diff revision 2)
>  :root[platform="mac"] .split-console-close-button-wrapper {
>    padding-inline-end: 3px;
>  }
>  
>  #split-console-close-button {
>    margin-inline-end: 0;

making this a margin: 0 seems to remove the overlap I was talking about

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8982967 [details]
Bug 1464939 - Part 2.Remove right margin in case of the platform is not 'mac'.

https://reviewboard.mozilla.org/r/248816/#review256336

::: devtools/client/themes/webconsole.css:1109
(Diff revision 2)
> -  padding: 4px 0;
> +  padding: 0;
>    margin: 0;
> -  margin-inline-end: -3px;
> +  margin-inline-end: 0px;

so I think the goal here was to align the close sidebar button with the close toolbox one; which now doesn't seem to be the case with this patch.

Comment 13

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8982966 [details]
Bug 1464939 - Part 1.Remove padding-inline-start and padding-inline-end style from the focusring of devtools's button and tab.

https://reviewboard.mozilla.org/r/248814/#review256334

Sorry for my late reply.

> it would be interesting to know why it was set in the first place.

Actually, this is not needed. I'm sorry , I made a mistake.

> making this a margin: 0 seems to remove the overlap I was talking about

The 'devtools-button' has margin:1px. As result of it, this button is overlapped. So I need to remove the margin of this button. I fixed this by using margin:0.

Comment 14

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8982967 [details]
Bug 1464939 - Part 2.Remove right margin in case of the platform is not 'mac'.

https://reviewboard.mozilla.org/r/248816/#review256336

> so I think the goal here was to align the close sidebar button with the close toolbox one; which now doesn't seem to be the case with this patch.

Yes, the purpose of this patch is setting the padding-inline-end:3px only if the platform is macOS, like bug 1455589. Other platform should drop this padding-inline-end. At the moment, sidebar close button's margin is -3px, so this patch will set this margin to 0px and add padding-inline-end style for macOS.

However, this patch contained unnecessary code which setting padding-top/padding-bottom and margin-inline-end to zero. So I modified it.

Comment 15

[Mantaroh Yoshinaga[:mantaroh]]

Oops. I can't request the review via mozreview due to "Error publishing: Bugzilla error: The flag type 'review' matches several flag types. You must specify the type id value to update or add a flag.". I will re-request the review in order to resolve this error.

Comment 16

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #15)
> Oops. I can't request the review via mozreview due to "Error publishing:
> Bugzilla error: The flag type 'review' matches several flag types. You must
> specify the type id value to update or add a flag.". I will re-request the
> review in order to resolve this error.

I think changing product might cause this problem. (see https://mozilla.logbot.info/developers/20180207#c14257247 and bug 1246831)

Comment 17

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1464939 - Part 1.Remove padding-inline-start and padding-inline-end style from the focusring of devtools's button and tab.

The devtool's focusring style will set the padding-inline-* to zero[1]. So
devtool's button which has padding will shrink the width when target element has
the focus. This patch will remove this style.

The tab style doesn't have a padding, so this change will not effect the tool tab.[2]

Furthermore, this will stretch the height of close button of split console.

[1] https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/devtools/client/themes/common.css#270-271
[2] https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/devtools/client/themes/toolbox.css#99

Review commit: https://reviewboard.mozilla.org/r/250892/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250892/

Comment 18

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1464939 - Part 2.Remove right margin in case of the platform is not 'mac'.

Review commit: https://reviewboard.mozilla.org/r/250894/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250894/

Comment 19

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8985404 [details]
Bug 1464939 - Part 1.Remove padding-inline-start and padding-inline-end style from the focusring of devtools's button and tab.

The devtool's focusring style will set the padding-inline-* to zero[1]. So
devtool's button which has padding will shrink the width when target element has
the focus. This patch will remove this style.

The tab style doesn't have a padding, so this change will not effect the tool tab.[2]

Furthermore, this will stretch the height of close button of split console.

[1] https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/devtools/client/themes/common.css#270-271
[2] https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/devtools/client/themes/toolbox.css#99

Review commit: https://reviewboard.mozilla.org/r/250892/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250892/

Comment 20

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8985405 [details]
Bug 1464939 - Part 2.Remove right margin in case of the platform is not 'mac'.

Review commit: https://reviewboard.mozilla.org/r/250894/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250894/

Comment 21

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8985404 [details]
Bug 1464939 - Part 1.Remove padding-inline-start and padding-inline-end style from the focusring of devtools's button and tab.

https://reviewboard.mozilla.org/r/250892/#review257504

Tested it and looks fine, thanks Mantaroh !

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8985405 [details]
Bug 1464939 - Part 2.Remove right margin in case of the platform is not 'mac'.

https://reviewboard.mozilla.org/r/250894/#review257506

Looks good

Comment 23

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8985404 [details]
Bug 1464939 - Part 1.Remove padding-inline-start and padding-inline-end style from the focusring of devtools's button and tab.

https://reviewboard.mozilla.org/r/250892/#review257504

Thank you for the review!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f64ce173505
Part 1.Remove padding-inline-start and padding-inline-end style from the focusring of devtools's button and tab.r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/5e37ebe9339e
Part 2.Remove right margin in case of the platform is not 'mac'.r=nchevobbe

Comment 25

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/7f64ce173505
https://hg.mozilla.org/mozilla-central/rev/5e37ebe9339e