1341045 - Button image does not fit into button in Responsive Design Mode

Status: VERIFIED FIXED

(DevTools :: Responsive Design Mode, defect, P1)

Description

[magicp]

Attachment: button-image-does-not-fit-into-button-in-responsive-design-mode.png

[Steps to reproduce]
1. Start latest Nightly (e.g. Build Id: 20170219030209)
2. Open Responsive Design Mode (Ctrl+Shift+M)
3. Confirm position of button images

[Actual Results]
Button image does not fit into button.

[Expected Results]
Button image fits into button.

Comment 1

[magicp]

Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=813c8c4770bc41bd69457e57e62b2bbe9ffeb855&tochange=6eb03c463039bb62bdfe4bf66cea8144c7cb83c0

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:ntim, could you look into this?

Comment 3

[Tim Nguyen :ntim]

Attachment: Bug 1341045 - Remove broken CSS overrides from responsive.html.

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

Comment 4

[Tim Nguyen :ntim]

Attachment: Bug 1341045 - Use checked class instead of introducing active class.

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

Comment 5

[Tim Nguyen :ntim]

Attachment: Bug 1341045 - Make sure RDM close icon is centered within button.

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

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8841223 [details]
Bug 1341045 - Make sure RDM close icon is centered within button.

https://reviewboard.mozilla.org/r/115502/#review117106

Thanks, this deduplcation seems fine.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8841221 [details]
Bug 1341045 - Remove broken CSS overrides from responsive.html.

https://reviewboard.mozilla.org/r/115498/#review117108

Thanks for working on this!  It's getting close, but I think a few more changes are needed to restore previous appearance.

::: devtools/client/responsive.html/index.css:74
(Diff revision 1)
>  .container {
>    background-color: var(--theme-toolbar-background);
>    border: 1px solid var(--theme-splitter-color);
>  }
>  
>  .toolbar-button {

`.devtools-button` now gets a background color on hover, which is different than previous behavior.  I think the RDM buttons don't want this (at least for this bug, let's just aim to restore previous functionality).

So, we'll need a rule for something like `.toolbar-button:hover` to clear the background, but it needs to override the `.devtools-button:empty:hover:not(:disabled)` rule.

The same thing happens with hover when checked, so another rule seems to be needed to clear that as well.

::: devtools/client/responsive.html/index.css
(Diff revision 1)
>  }
>  
>  .toolbar-button {
> -  margin: 1px 3px;
> -  width: 16px;
> +  margin: 0;
> +  padding: 0;
> -  height: 16px;

It seems like there are more styles needed to restore the previous appearance:

```
border: none;
```

After adding this, it feels like the viewport rotate button is no longer vertically centered...  Maybe reduce the height on `.viewport-toolbar` by 2px to 16px?

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8841222 [details]
Bug 1341045 - Use checked class instead of introducing active class.

https://reviewboard.mozilla.org/r/115500/#review117130

Thanks, this seems like good simplification.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:ntim, it would be great to fix this before the release on Monday.  Do you have time to work on it, or should I finish it up?

Comment 10

[Tim Nguyen :ntim]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #9)
> :ntim, it would be great to fix this before the release on Monday.  Do you
> have time to work on it, or should I finish it up?

It would be great if you could finish it up. I don't have time this month.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1341045 - Remove broken CSS overrides from responsive.html.

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

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1341045 - Use checked class instead of introducing active class.

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

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1341045 - Make sure RDM close icon is centered within button.

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

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8843372 [details]
Bug 1341045 - Make sure RDM close icon is centered within button.

https://reviewboard.mozilla.org/r/117136/#review118744

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8843371 [details]
Bug 1341045 - Use checked class instead of introducing active class.

https://reviewboard.mozilla.org/r/117134/#review118748

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8843370 [details]
Bug 1341045 - Remove broken CSS overrides from responsive.html.

https://reviewboard.mozilla.org/r/117132/#review118750

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f67c47f25e6c
Remove broken CSS overrides from responsive.html. r=jryans
https://hg.mozilla.org/integration/autoland/rev/a8457e6674a2
Use checked class instead of introducing active class. r=jryans
https://hg.mozilla.org/integration/autoland/rev/cb3ac86ceeb8
Make sure RDM close icon is centered within button. r=jryans

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f67c47f25e6c
https://hg.mozilla.org/mozilla-central/rev/a8457e6674a2
https://hg.mozilla.org/mozilla-central/rev/cb3ac86ceeb8

Comment 19

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 54.0a1 (2017-02-20) (64-bit) on WIndows 7 , 64 Bit!

This bug's fix is verified with latest Developer Edition (Aurora)!

Build ID          20170309004016
User Agent        Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0

[bugday-20170308]

Comment 20

[Pavithra Roselin]

[bugday-20170419]

Status: Fixed and verified
Browser: Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0

The issue is no longer reproducible on Firefox Aurora 
Tests were performed in Ubuntu 16.04.2 x64