Closed Bug 1401847 Opened 2 years ago Closed 2 years ago

Layout Grid colors should be restored after reload, even if grid is unchecked

Categories

(DevTools :: Inspector, defect, P2)

57 Branch
defect

Tracking

(firefox57 wontfix, firefox58 wontfix, firefox59 wontfix, firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: Harald, Assigned: ewright)

References

(Blocks 1 open bug)

Details

(Keywords: DevAdvocacy, Whiteboard: [DevRel:P1])

Attachments

(1 file)

Via @SaraSoueidan: https://twitter.com/SaraSoueidan/status/910567472604024832

STR:
- Inspect Grid
- Change color for a grid
- Don't check the grid to be shown
- Reload

ER: Changed color should be restored
AR: Color is reset to default

This STR works as expected when the grid is actually checked to be visible; but not when the grid is unchecked.
Blocks: dt-grid
Yes, the changes a user makes to a color should always save state — always remain changed.
Keywords: DevAdvocacy
Whiteboard: [DevRel:P1]
We really need to fix this bug.
Priority: -- → P2
Hiya,

I am currently looking for bugs to fix as part of my Open Source Development module at Coventry University and I am interested in developing this bug.

Please could you assign this task to me and give me more information.

This is my first bug fix and any help would be appreciated.

Thank you.
Assignee: nobody → teddsr
Status: NEW → ASSIGNED
Hi Rayann,

Take a look at https://bugzilla.mozilla.org/show_bug.cgi?id=1342310 which implemented the persistence of the grid highlighter on reload. You will mainly be working in https://searchfox.org/mozilla-central/source/devtools/client/inspector/shared/highlighters-overlay.js#48.

I would store the state of all grids in the page and restore if the corresponding grid node is still on the page after reload. You can see that we currently restore the highlighter checked state. It should be similar for restoring the color.

My current recommended setup is here - https://github.com/gabrielluong/devtools-getting-started.

Thanks,
Gabriel
Hi Rayann, 

Just wanted to see how you have been doing on this bug. There's no pressure if you haven't gotten started yet. Just wanted to see if there's anyway I can help.

Thanks,
Gabriel
Flags: needinfo?(teddsr)
Gonna assign this to myself since I haven't heard back.
Assignee: teddsr → gl
Flags: needinfo?(teddsr)
Assignee: gl → nobody
Status: ASSIGNED → NEW
Assignee: nobody → gl
Status: NEW → ASSIGNED
Assignee: gl → nobody
Status: ASSIGNED → NEW
Assignee: nobody → ewright
After discussion with :jensimmons, We'd like be able to save a palette per domain. In essence, on one website, the user picks a custom color, or a few, and those settings stay forever for that domain. On a new website, it resets to default.
Comment on attachment 8951637 [details]
Bug 1401847 - Custom layout grid colors should be restored per domain.

https://reviewboard.mozilla.org/r/220926/#review226864

I think we should start adding unit tests

::: devtools/client/inspector/grids/grid-inspector.js:157
(Diff revision 1)
>     * @param  {String} fallbackColor
>     *         The color to use if no color could be found for the node front.
>     * @return {String} color
>     *         The color to use.
>     */
> -  getInitialGridColor(nodeFront, fallbackColor) {
> +  getInitialGridColor(nodeFront, customColor, fallbackColor) {

Update the JSDoc with the customColor param

::: devtools/client/inspector/grids/grid-inspector.js:294
(Diff revision 1)
>    async updateGridPanel() {
>      // Stop refreshing if the inspector or store is already destroyed.
>      if (!this.inspector || !this.store) {
>        return;
>      }
> +    let hostname = extractHostname(this.inspector.target.url);

Add a new line before this.

::: devtools/client/inspector/grids/grid-inspector.js:332
(Diff revision 1)
>            // closing.
>            return;
>          }
>        }
>  
> +      let customColors = await asyncStorage.getItem("devtoolsGridColors");

We should put this with the hostname declaration

::: devtools/client/inspector/grids/grid-inspector.js:332
(Diff revision 1)
>            // closing.
>            return;
>          }
>        }
>  
> +      let customColors = await asyncStorage.getItem("devtoolsGridColors");

Looking at our async storage usage, I think we can drop the devtools prefix and maybe use gridInspectorHostColors as the key instead.

::: devtools/client/inspector/grids/grid-inspector.js:468
(Diff revision 1)
>     */
> -  onSetGridOverlayColor(node, color) {
> +  async onSetGridOverlayColor(node, color) {
>      this.store.dispatch(updateGridColor(node, color));
>      let { grids } = this.store.getState();
> +    let hostname = extractHostname(this.inspector.target.url);
> +    let customGridColors = await asyncStorage.getItem("devtoolsGridColors");

I am wondering if we could do 

await blah() || {};

::: devtools/client/inspector/grids/grid-inspector.js:483
(Diff revision 1)
> +      }
>  
> -    // If the grid for which the color was updated currently has a highlighter, update
> +      // If the grid for which the color was updated currently has a highlighter, update
> -    // the color.
> +      // the color.
> -    for (let grid of grids) {
>        if (grid.nodeFront === node && grid.highlighted) {

This can actually go into the previous if statement and be simplified to 

if (grid.highlighted) { }
Attachment #8951637 - Flags: review?(gl)
To clarify, the request was for all pages on a site to have the same palette. However subdomains are considered their own site, and have their own palette. 

`this.example.com` will have a different palette than `example.com`. But `example.com/users/` will have the same palette as `example.com/posts/`

Also, the way this is written, all data urls result in a hostname of "data", so they will also have the same palette.
Attachment #8951637 - Flags: review?(gl) → review?(jdescottes)
Status: NEW → ASSIGNED
Comment on attachment 8951637 [details]
Bug 1401847 - Custom layout grid colors should be restored per domain.

https://reviewboard.mozilla.org/r/220926/#review228966

Looks good and works well, thanks for the patch Erica! 
I would like to see another version without the new url helper and with a more complete test.

::: devtools/client/inspector/grids/grid-inspector.js:297
(Diff revision 2)
>      // Stop refreshing if the inspector or store is already destroyed.
>      if (!this.inspector || !this.store) {
>        return;
>      }
>  
> +    let hostname = extractHostname(this.inspector.target.url);

You should be able to use parseURL from source-utils here: 

https://searchfox.org/mozilla-central/source/devtools/client/shared/source-utils.js#52

  let { hostname } = parseURL(this.inspector.target.url);

The only issue is that the hostname for data:// urls is an empty string when using parseURL. So you would either need to adapt your test or to fallback on the protocol if hostname is empty.

::: devtools/client/inspector/grids/grid-inspector.js:336
(Diff revision 2)
>            // closing.
>            return;
>          }
>        }
>  
> +      let colorsForHost = customColors[hostname] ? customColors[hostname][i] : undefined;

nit: singular? we are passing a single color string, colorsForHost -> customColor (to avoid colorForHostAndForIndex :) )

::: devtools/client/inspector/grids/test/browser_grids_persist-color-palette.js:23
(Diff revision 2)
> +  </div>
> +`;
> +
> +add_task(function* () {
> +  // "data" ends up as the hostname of the test_uri when written as a data url
> +  yield asyncStorage.setItem("gridInspectorHostColors", {"data": ["#333000"]});

This test checks that we restore colors from storage when opening the panel. We should also cover:
- persisting color updates (open page, change a grid color, close and reopen inspector, check the color is still correct)
- different domains use different colors

For the first one I think we should expand your test. Look at browser_grids_grid-list-color-picker-on-RETURN.js for setting the color. And at browser_grids_accordion-state.js for closing/reopening the toolbox. This way we can avoid relying on asyncStorage.setItem(...), which links the test too much with the current implementation.

For the second one we could use a page served on the test server that responds to http://example.com and another served on a subdomain, but that can be done in a followup bug.

::: devtools/client/inspector/grids/test/head.js:45
(Diff revision 2)
>    info("Applying the change");
>    spectrum.updateUI();
>    spectrum.onChange();
>  });
> +
> +registerCleanupFunction(function* () {

nit use async rather than function*

::: devtools/client/inspector/grids/utils/utils.js:52
(Diff revision 2)
> +function extractHostname(url) {
> +  let hostname;
> +
> +  if (url.indexOf("://") > -1) {
> +    hostname = url.split("/")[2];
> +  } else {
> +    hostname = url.split("/")[0];
> +  }
> +
> +  hostname = hostname.split(":")[0];
> +  hostname = hostname.split("?")[0];
> +  return hostname;
> +}
> +

You should be able to remove this if you use parseURL as suggested above.
Attachment #8951637 - Flags: review?(jdescottes)
Comment on attachment 8951637 [details]
Bug 1401847 - Custom layout grid colors should be restored per domain.

https://reviewboard.mozilla.org/r/220926/#review228966

> nit use async rather than function*

Do you know the syntax for that? I can't seem to find examples. `registerCleanupFunction(async function () {...}` fails.
Comment on attachment 8951637 [details]
Bug 1401847 - Custom layout grid colors should be restored per domain.

https://reviewboard.mozilla.org/r/220926/#review228966

> Do you know the syntax for that? I can't seem to find examples. `registerCleanupFunction(async function () {...}` fails.

:julian never mind, I figured it out, it needed an `await` instead of a `yield`. Can you explain why you prefer `async` over `function*`? And would you like the `add_task` functions to use `await/async` too? All the other tests save one are written using `function*` - are there plans to change these?
Comment on attachment 8951637 [details]
Bug 1401847 - Custom layout grid colors should be restored per domain.

https://reviewboard.mozilla.org/r/220926/#review230742

::: devtools/client/inspector/grids/test/head.js:45
(Diff revision 2)
>    info("Applying the change");
>    spectrum.updateUI();
>    spectrum.onChange();
>  });
> +
> +registerCleanupFunction(function* () {

In general we should avoid adding new code that uses function* / yield if we can. 

We are moving our code to use async/await instead of generator functions + yield.  function*/yield are in general used in conjunction with Task.jsm which is a custom piece of code to emulate async/await. async/await is a native language feature, so it makes sense to try to migrate to this now that it is available to us.

There have been a few bugs logged already to migrate part of the code such as https://bugzilla.mozilla.org/show_bug.cgi?id=1437828. Of course this will take time so not all the code under migration yet.

Good point about the add_task in the test, it would be great to use async/await there too.
Comment on attachment 8951637 [details]
Bug 1401847 - Custom layout grid colors should be restored per domain.

https://reviewboard.mozilla.org/r/220926/#review230990

The patch looks good, thanks Erica! Unfortunately it makes grid tests fail intermittently. 
We need to fix the test helpers to wait for the grid panel to be fully initialized.

::: devtools/client/inspector/grids/grid-inspector.js:301
(Diff revision 3)
>      }
> +    let currentUrl = this.inspector.target.url;
> +    // Get the hostname, if there is no hostname, fall back on protocol
> +    // ex: `data:` uri, and `about:` pages
> +    let hostname = parseURL(currentUrl).hostname || parseURL(currentUrl).protocol;
> +    let customColors = await asyncStorage.getItem("gridInspectorHostColors") || {};

Unfortunately, the patchs add more asynchronous work in this method, which makes several grid tests fail intermittently.

To fix is we need to wait for "updateGridPanel" to be completely finished before starting the tests. 

We already do something similar for the BoxModel at https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/devtools/client/inspector/test/shared-head.js#56-58 . The goal here would be to fire an event at the end of updateGridPanel (this.inspector.emit("grid-panel-updated") for instance) and to listen for this event in the helper linked above.

You can use `./mach test browser_grids_persist-color-palette.js --headless --verify` in order to run a single test a lot of times. It usually helps detecting intermittent issues such as this one.

This architectural issue existed before your patch, but the async work was probably too fast too make the tests fail.

::: devtools/client/inspector/grids/grid-inspector.js:480
(Diff revision 3)
> +    // ex: `data:` uri, and `about:` pages
> +    let hostname = parseURL(currentUrl).hostname || parseURL(currentUrl).protocol;
> +    let customGridColors = await asyncStorage.getItem("gridInspectorHostColors") || {};
> +
> +    for (let grid of grids) {
> +      // Update the custom color for the grid in this position.

nit: we could move this comment above line 482 or 485
Attachment #8951637 - Flags: review?(jdescottes)
Comment on attachment 8951637 [details]
Bug 1401847 - Custom layout grid colors should be restored per domain.

https://reviewboard.mozilla.org/r/220926/#review231056

Looks good Erica, thanks! One last nit to address, then let's push to try and lang if try is green.

::: devtools/client/inspector/test/shared-head.js:59
(Diff revision 4)
>    if (id === "computedview" || id === "layoutview") {
>      // The layout and computed views should wait until the box-model widget is ready.
>      let onBoxModelViewReady = inspector.once("boxmodel-view-updated");
> +    // The layout view also needs to wait for the grid panel to be fully updated.
> +    let onGridPanelReady = id === "layoutview" ?
> +    inspector.once("grid-panel-updated") : Promise.resolve();

nit: could we indent with 2 additional spaces here?
Attachment #8951637 - Flags: review?(jdescottes) → review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6e462b0ef169
Custom layout grid colors should be restored per domain. r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/6e462b0ef169
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
No longer blocks: top-inspector-bugs
You need to log in before you can comment on or make changes to this bug.