1308694 - Introduce a tooltip for the waterfall graph displaying timings

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

Support tooltip when hovering on request item's waterfall graph including onLoad, onContentLoad.

Comment 1

[Ricky Chien [:rickychien] (inactive)]

This is a follow-up for drawWaterfallBackground component (bug 1308504)

Comment 2

[Julien Cristau [:jcristau]]

Mass wontfix for bugs affecting firefox 52.

Comment 3

[Vangelis Katsikaros]

Attachment: demo-tooltip.png

Comment 4

[Vangelis Katsikaros]

I initially thought I could tweak this without input, but it seems I was wrong :/ 

So far, as a demo/tour of the related code, I have managed to tweak the waterfall tooltip, for each request (see https://bugzilla.mozilla.org/attachment.cgi?id=8858872). However, this is not the goal.

To call `getDisplayedTimingMarker(state, "firstDocumentDOMContentLoadedTimestamp")` or `getDisplayedTimingMarker(state, "firstDocumentLoadTimestamp"),` (like in http://searchfox.org/mozilla-central/rev/a7334b2896ed720fba25800e11e24952e6037d77/devtools/client/netmonitor/src/components/status-bar.js#77) I need access to the `state`. However, I am not sure how to get access to this

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Attachment: chromium-waterfall-tooltip.png

Good job! First off, thank you for picking up this work.

There is a story that at the beginning we want to mimic Chromium's UI to show the timing detail when hovering on a particular water timeline (see as attachment).

There is no UX spec and no discussion relate to implementation. But we're working on developer tool so we're happy to see someone wants to jump in and do any improvements. 

I think it's your call to design the look and feel, and your demo-tooltip looks good to me :)

I don't really understand your question, where are you trying to access to the `state`. Could you paste your code snippet or patch?

btw, feel free to reach out me on public devtools Slack (https://devtools-html.slack.com/messages) and join #netmonitor channel.

Thanks!

Comment 6

[Vangelis Katsikaros]

Attachment: status_bar_ff55.png

Comment 7

[Vangelis Katsikaros]

Ricky thanks for the comments and sorry for not clarifying.

> I don't really understand your question, where are you trying to access to the `state`. Could you paste your code snippet or patch?

Initially, I thought the goal was to show a tooltip with `DOMContentLoaded` and `Load`, like in the FF55's network panel status bar (https://bugzilla.mozilla.org/attachment.cgi?id=8859242). For these two metrics, it I understand I'd need `getDisplayedTimingMarker(state, "firstDocumentDOMContentLoadedTimestamp")` or `getDisplayedTimingMarker(state, "firstDocumentLoadTimestamp"),` (like in http://searchfox.org/mozilla-central/rev/a7334b2896ed720fba25800e11e24952e6037d77/devtools/client/netmonitor/src/components/status-bar.js#77) which means I need access to the `state`. Thus my need to do something with the `state`. (This also puzzled me since `DOMContentLoaded` and `Load` are for the DOM and not per request, so this would mean that the tooltip would containt the same data for each request)

However, it seems that I was wrong, the goal is different :) 

> I think it's your call to design the look and feel, and your demo-tooltip looks good to me :)
If the demo tooltop is of interest, I can submit the patch. It's not close to `chromium-waterfall-tooltip.png` but it's a start. Perhaps, I could reuse the tooltip-like image preview (not sure how it's called) to draw something prettier. What do you think?

Comment 8

[Tim Nguyen :ntim]

(In reply to Vangelis Katsikaros from comment #7)
The "state" you're mentioning is stored in the redux store.
You can access it by using react-redux's connect() function (like in [0]). The data returned in the first function will be passed in as props for the react component that is connected. Once the data is passed as a prop for the react component, you can also pass it down to child components (without connect()).

However, if you're not dealing with a react component, you can access the state by using `window.gStore.getState();`, or pass in actions/state from the react component to the non-react module.

[0]: http://searchfox.org/mozilla-central/rev/a7334b2896ed720fba25800e11e24952e6037d77/devtools/client/netmonitor/src/components/status-bar.js#77

> > I think it's your call to design the look and feel, and your demo-tooltip looks good to me :)
> If the demo tooltop is of interest, I can submit the patch. It's not close
> to `chromium-waterfall-tooltip.png` but it's a start. Perhaps, I could reuse
> the tooltip-like image preview (not sure how it's called) to draw something
> prettier. What do you think?

If we want a full chromium-like tooltip, I think it would be easier to get bug 1306291 fixed, and then simply load devtools/client/netmonitor/src/components/timings-panel.js into the tooltip. Otherwise, a simple tooltip like attachment 8858872 [details] should do. Not sure what's Ricky's opinion though.

Comment 9

[Ricky Chien [:rickychien] (inactive)]

Yep. ntim has answered the question entirely! Here is react-redux official doc http://redux.js.org/docs/basics/UsageWithReact.html.

> If we want a full chromium-like tooltip, I think it would be easier to get
> bug 1306291 fixed, and then simply load
> devtools/client/netmonitor/src/components/timings-panel.js into the tooltip.
> Otherwise, a simple tooltip like attachment 8858872 [details] should do. Not
> sure what's Ricky's opinion though.

Feel free to file a new bug for supporting chromium-like tooltip. I agreed bug 1306291 is the shortcut to support timings-panel tooltip. The demo-tooltip looks good to me, I think we can ship it first to gain this handy tooltip feature in short-term.

Comment 10

[Vangelis Katsikaros]

Attachment: Bug 1308694 - Introduce a tooltip for the waterfall graph displaying timings.

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

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Ricky Chien [:rickychien] from comment #9)
> Feel free to file a new bug for supporting chromium-like tooltip. I agreed
> bug 1306291 is the shortcut to support timings-panel tooltip. The
> demo-tooltip looks good to me, I think we can ship it first to gain this
> handy tooltip feature in short-term.
Yep, agree.

Honza

Comment 13

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8859450 [details]
Bug 1308694 - Introduce a tooltip for the waterfall graph displaying timings.

https://reviewboard.mozilla.org/r/130856/#review134276

Great work vkatsikaros! I like that.

::: devtools/client/netmonitor/src/components/request-list-column-waterfall.js:65
(Diff revision 1)
>    const { eventTimings, totalTime, fromCache, fromServiceWorker } = item;
>    let boxes = [];
> +  let tooltip = [];
>  
>    if (fromCache || fromServiceWorker) {
> -    return boxes;
> +    return { boxes: boxes, tooltip: tooltip };

nit:  `{ boxes, tooltip };`

This is shorthand property name in ES2015

https://developer.mozilla.org/zh-TW/docs/Web/JavaScript/Reference/Operators/Object_initializer

::: devtools/client/netmonitor/src/components/request-list-column-waterfall.js:96
(Diff revision 1)
>        title: text
>      }, text));
> +    tooltip.push(L10N.getFormatStr("netmonitor.waterfall.tooltip.total", totalTime));
>    }
>  
> -  return boxes;
> +  return { boxes: boxes, tooltip: tooltip.join(", ") };

nit: use shorthand property name instead.

Comment 14

[Vangelis Katsikaros]

Comment on attachment 8859450 [details]
Bug 1308694 - Introduce a tooltip for the waterfall graph displaying timings.

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

Comment 15

[Vangelis Katsikaros]

Comment on attachment 8859450 [details]
Bug 1308694 - Introduce a tooltip for the waterfall graph displaying timings.

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

Comment 16

[Vangelis Katsikaros]

Rebased the previous patch and pushed.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/92b1623491bf
Introduce a tooltip for the waterfall graph displaying timings. r=rickychien

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/92b1623491bf

Comment 19

[Francesco Lodolo [:flod]]

Comment on attachment 8859450 [details]
Bug 1308694 - Introduce a tooltip for the waterfall graph displaying timings.

https://reviewboard.mozilla.org/r/130856/#review137874

::: devtools/client/netmonitor/src/components/request-list-column-waterfall.js:99
(Diff revision 3)
>        }, title)
>      );
> +    tooltip.push(L10N.getFormatStr("netmonitor.waterfall.tooltip.total", totalTime));
>    }
>  
> -  return boxes;
> +  return { boxes, tooltip: tooltip.join(", ") };

Do not do this. You're imposing a structure, that works for English, to all languages that are trying to localize devtools.

For example, I would not use an uppercase letter after a comma in my own language.

Suggested read: https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices

Comment 20

[Vangelis Katsikaros]

Comment on attachment 8859450 [details]
Bug 1308694 - Introduce a tooltip for the waterfall graph displaying timings.

https://reviewboard.mozilla.org/r/130856/#review137874

> Do not do this. You're imposing a structure, that works for English, to all languages that are trying to localize devtools.
> 
> For example, I would not use an uppercase letter after a comma in my own language.
> 
> Suggested read: https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices

Francesco, thanks for the input, I overlooked this completely. I understadnt that the latin comma might even be a valid option for some languages. Opened https://bugzilla.mozilla.org/show_bug.cgi?id=1360902 to tackle this bug

Comment 21

[Nazir Ahmed Sabbir [:NaSb]]

I reproduced this issue using Nightly 52.0a1 (2016-10-07), Build ID: 20161007030207 on Linux x64.

I can confirm this bug is now verified as fixed on Latest Firefox Nightly 55.0a1 (2017-04-30) (64-bit)

Build ID: 20170430100638
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0
OS: Linux 4.4.0-72-generic; Elementary OS 0.4