Closed Bug 1459581 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: Application Panel, defect)

Product:
DevTools
Component:
Application Panel
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox62 verified, firefox64 verified, firefox65 verified, firefox66 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- verified
firefox64 --- verified
firefox65 --- verified
firefox66 --- verified

People

(Reporter: ladybenko, Assigned: ladybenko)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Screen Shot 2018-05-04 at 18.22.07.png
41.61 KB, image/png
Details
Bug 1459581 - CSS and class names refactor, and fix UI glitch when sw scope is too long.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details 
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.
Status: NEW → ASSIGNED
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.
Attachment #8974325 - Flags: review?(jdescottes)
See Also: → 1460606
Depends on: 1460606
See Also: 1460606 → 
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 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
Attachment #8974325 - Flags: review?(jdescottes) → review+
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)
The patch needs a small rebase before it can land :) Ping me when ready so that I can land it.
Flags: needinfo?(balbeza)
Flags: needinfo?(balbeza)
Keywords: checkin-needed
Pushed by jdescottes@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/35f42e0346a7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Depends on: 1464354
Product: Firefox → DevTools
Flags: qe-verify+
Hi, Does anybody have any Repro Steps ? or maybe a reduced case for this issue, I'm not sure how I can verify it.
Flags: needinfo?(balbeza)
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.
Flags: needinfo?(balbeza)
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).
Status: RESOLVED → VERIFIED
status-firefox62: fixed → verified
status-firefox64: --- → verified
status-firefox65: --- → verified
status-firefox66: --- → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: