1459581 - UI glitch when the service worker's scope is too long

Status: VERIFIED FIXED

(DevTools :: Application Panel, defect)

Description

[Belén [:ladybenko]]

Attachment: Screen Shot 2018-05-04 at 18.22.07.png

There's no separation between the worker's scope and the Unregister button when the scope is too long. We should clip the scope or span over multiple lines (Chrome does the former), and always keep the margin between the text and the button.

Comment 1

[Belén [:ladybenko]]

Attachment: Bug 1459581 - CSS and class names refactor, and fix UI glitch when sw scope is too long.

This was a good time to revamp of the CSS layout we were using in the
Worker component. Changes that have been made:

- Use CSS Grid to layout the Worker component
- Rename classes to use BEM style (this is compatible with the current
  CSS guidelines for the DevTools).
- Use classes with the js- prefix for JS hooks
- Rename classes / use js- class names for hooks in other components,
  so everything is consistent.
- Some HTML markup has been fixed, where it wasn't in conflict with
  the current styling.

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

Comment 2

[Belén [:ladybenko]]

Comment on attachment 8974325 [details]
Bug 1459581 - CSS and class names refactor, and fix UI glitch when sw scope is too long.

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

Comment 3

[Belén [:ladybenko]]

Comment on attachment 8974325 [details]
Bug 1459581 - CSS and class names refactor, and fix UI glitch when sw scope is too long.

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

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 8974325 [details]
Bug 1459581 - CSS and class names refactor, and fix UI glitch when sw scope is too long.

https://reviewboard.mozilla.org/r/242654/#review248822

Thanks for the patch Belén! I like the guidelines and general direction, especially having dedicated classes for js hooks. The CSS also looks already much more maintainable. Maybe we should add a comment in application.css to indicate which conventions the panel follows on top of the DevTools ones but that should be done in a follow up.

I am clearing the review flag for now mostly for the test failure of browser_parsable_css.js which we need to address.
If we change this test, let's keep it in a separate changeset as we will probably need another reviewer than myself to validate it.

::: commit-message-0cd10:6
(Diff revision 3)
> +- Use CSS Grid to layout the Worker component
> +- Rename classes to use BEM style (this is compatible with the current
> +  CSS guidelines for the DevTools).
> +- Use classes with the js- prefix for JS hooks
> +- Rename classes / use js- class names for hooks in other components,
> +  so everything is consistent.
> +- Some HTML markup has been fixed, where it wasn't in conflict with
> +  the current styling.

In this case it could have been nice to keep separate commits for some of the points listed here.

If we keep a single commit for this, the commit's first line should probably mention BEM-style/Grid layout/semantic markup, rather than the UI fix. In general the commit message should explain what changed more than the original intent.

::: devtools/client/application/src/components/App.css:26
(Diff revision 3)
>    overflow: auto;
>    display: flex;
>    flex-direction: column;
>  }
>  
> -.application.empty {
> +.application--empty {

It seems that using "--" confuses the following mochitest:

https://searchfox.org/mozilla-central/source/browser/base/content/test/static/browser_parsable_css.js

It parses CSS texts using regexp and currently thinks this "--empty" is a CSS variable. (see https://searchfox.org/mozilla-central/rev/eb6c5214a63e20a3fff455e92c876287a8d2e188/browser/base/content/test/static/browser_parsable_css.js#265 )
Here we either fix the test to stop flagging BEM-style states as css variables. Or workaround (either by whitelisting or not using --empty) and log a followup to fix the test.

::: devtools/client/application/src/components/Worker.css:21
(Diff revision 3)
> -.service-worker-container {
> -  margin-bottom: 20px;
> +
> +.worker {
> +  display: grid;
> +  grid-template-rows: auto auto auto;
> +  grid-template-columns: auto 1fr;
>    width: 100%;

removing the max-width makes the button hard to find on wide screens, but this is probably something we can iterate over with UX.

::: devtools/client/application/src/components/Worker.css:26
(Diff revision 3)
>    width: 100%;
> -  max-width: 600px;
> -  position: relative;
> +  grid-column-gap: 0;
> +  padding: 1rem 0;
> +
>    line-height: 1.5;
> -  font-size: 13px;
> +  font-size: 1.2rem;

Linux has a way bigger default font, which we correct in some specific spots in the CSS:

https://searchfox.org/mozilla-central/search?q=%5Bplatform%3D%22linux%22%5D&case=true&regexp=false&path=devtools

If we want to use rem in the application panel we should apply a smaller font-size on the root element of our panel (80% seems to be the most commonly used ratio to get linux to font-sizes similar to other OSes)

::: devtools/client/application/src/components/Worker.js:126
(Diff revision 3)
>          "data-standalone": true
>        },
>          Strings.GetStringFromName("unregister"))
>        : null;
>  
>      const debugLinkDisabled = this.isRunning() ? "" : "disabled";

Is this standalone "disabled" class compliant with BEM guidelines?

::: devtools/client/application/src/components/Worker.js:153
(Diff revision 3)
> -        { className: "service-worker-source" },
> -        span({ className: "service-worker-meta-name" }, "Source"),
> -        span({ className: "js-source-url", title: worker.scope },
> -          this.formatSource(worker.url)),
> +      dl(
> +        { className: "worker__data" },
> +        dt({ className: "worker__meta-name" }, "Source"),
> +        dd({ title: worker.scope },
> +            span({ className: "js-source-url" }, this.formatSource(worker.url)),
> -        debugLink),
> +            debugLink),

This makes worker.scope also the title of the debug link (unless debug is deactivated, in which case it has its own title). Maybe the title should rather be on the span for js-source-url ?

(also realized we used the wrong title: this should be the worker url)

::: devtools/client/application/src/components/WorkerListEmpty.js:40
(Diff revision 3)
>    openDocumentation() {
>      this.props.serviceContainer.openWebLink(DOC_URL);
>    }
>  
>    render() {
> -    return div(
> +    /* TODO: the divs in here shoud be paragraphs */

nit: typo "shoud" -> "should".

Maybe give an indication of what is blocking this? If there are clear actions to take, we usually open a followup bug, and mention it in the TODO comment.

Comment 5

[Belén [:ladybenko]]

Comment on attachment 8974325 [details]
Bug 1459581 - CSS and class names refactor, and fix UI glitch when sw scope is too long.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242654/diff/3-4/

Comment 6

[Belén [:ladybenko]]

Comment on attachment 8974325 [details]
Bug 1459581 - CSS and class names refactor, and fix UI glitch when sw scope is too long.

https://reviewboard.mozilla.org/r/242654/#review248822

> removing the max-width makes the button hard to find on wide screens, but this is probably something we can iterate over with UX.

This is what Chrome is currently doing, so I opted for that; but IMO, as you said, this is something that should get decided in the UX review.

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8974325 [details]
Bug 1459581 - CSS and class names refactor, and fix UI glitch when sw scope is too long.

https://reviewboard.mozilla.org/r/242654/#review250526

Looks good to me, thanks!
Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdd9fc28f8c72a9c24048b4495cdc2722d9d3680

Comment 8

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 2e9f8c0679391c37e097379f0a7bce934c228174 -d 99a063f6a3f2: rebasing 464092:2e9f8c067939 "Bug 1459581 - CSS and class names refactor, and fix UI glitch when sw scope is too long. r=jdescottes" (tip)
merging devtools/client/application/src/components/WorkerListEmpty.js
merging devtools/client/application/test/head.js
warning: conflicts while merging devtools/client/application/src/components/WorkerListEmpty.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 9

[Julian Descottes [:jdescottes]]

The patch needs a small rebase before it can land :) Ping me when ready so that I can land it.

Comment 10

[Belén [:ladybenko]]

Comment on attachment 8974325 [details]
Bug 1459581 - CSS and class names refactor, and fix UI glitch when sw scope is too long.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242654/diff/4-5/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/35f42e0346a7
CSS and class names refactor, and fix UI glitch when sw scope is too long. r=jdescottes

Comment 12

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/35f42e0346a7

Comment 14

[Rares Doghi, Desktop QA]

Hi, Does anybody have any Repro Steps ? or maybe a reduced case for this issue, I'm not sure how I can verify it.

Comment 15

[Julian Descottes [:jdescottes]]

STRs:
- enable application panel in about:config (devtools.application.enabled to true)
- open devtools
- go to https://serviceworke.rs/strategy-network-or-cache_demo.html
- open application panel (you should see a service worker)
- resize the window width until the name of the service worker shows an ellipsis (and is not overflowing under the unregister button, which is what the bug was about).

Note that this panel and service worker debugging is not actively maintained/worked on at the moment since the project is on hold. We should probably clear the qa verify flags from all bugs in the Application Panel component.

Comment 16

[Rares Doghi, Desktop QA]

Hi Julian, thanks for the steps, I can confirm this issue Verified as Fixed in Firefox Release 64.0, Beta 65.0b6 and Nightly 66.0a1 (2018-12-19).