Introduce a tooltip for the waterfall graph displaying timings

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Developer Tools: Netmonitor
P1
normal
VERIFIED FIXED
8 months ago
29 days ago

People

(Reporter: rickychien, Assigned: Vangelis Katsikaros)

Tracking

(Blocks: 2 bugs, {dev-doc-needed, good-first-bug})

Trunk
Firefox 55
dev-doc-needed, good-first-bug
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox52 wontfix, firefox55 verified)

Details

(Whiteboard: [netmonitor-reserve])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

8 months ago
Support tooltip when hovering on request item's waterfall graph including onLoad, onContentLoad.
(Reporter)

Comment 1

8 months ago
This is a follow-up for drawWaterfallBackground component (bug 1308504)
(Reporter)

Updated

8 months ago
Blocks: 1307743
No longer blocks: 1308504
Depends on: 1308504

Updated

8 months ago
Whiteboard: [netmonitor]

Updated

8 months ago
Flags: qe-verify+
QA Contact: ciprian.georgiu

Updated

8 months ago
Priority: -- → P2

Updated

4 months ago
Priority: P2 → P1

Updated

4 months ago
Priority: P1 → P3
Whiteboard: [netmonitor] → [netmonitor-reserve]

Updated

2 months ago
Blocks: 1348737
No longer blocks: 1307743
Priority: P3 → P2
Whiteboard: [netmonitor-reserve] → [netmonitor]
Mass wontfix for bugs affecting firefox 52.
status-firefox52: affected → wontfix

Updated

2 months ago
Keywords: good-first-bug
(Assignee)

Updated

a month ago
Assignee: nobody → vkatsikaros

Updated

a month ago
Status: NEW → ASSIGNED
Priority: P2 → P3
Whiteboard: [netmonitor] → [netmonitor-reserve]
(Assignee)

Comment 3

a month ago
Created attachment 8858872 [details]
demo-tooltip.png
(Assignee)

Comment 4

a month ago
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
Flags: needinfo?(rchien)
(Reporter)

Comment 5

a month ago
Created attachment 8859231 [details]
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!
Flags: needinfo?(rchien)
(Assignee)

Comment 6

a month ago
Created attachment 8859242 [details]
status_bar_ff55.png
(Assignee)

Comment 7

a month ago
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?
Flags: needinfo?(rchien)

Comment 8

a month ago
(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.
(Reporter)

Comment 9

a month ago
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.
Flags: needinfo?(rchien)
Comment hidden (mozreview-request)
(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
(Assignee)

Updated

a month ago
Blocks: 1357652
(Assignee)

Updated

a month ago
Duplicate of this bug: 859053
(Reporter)

Comment 13

a month ago
mozreview-review
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.
Attachment #8859450 - Flags: review?(rchien) → review+
Comment hidden (mozreview-request)

Updated

a month ago
Keywords: dev-doc-needed
Comment hidden (mozreview-request)
(Assignee)

Comment 16

a month ago
Rebased the previous patch and pushed.

Comment 17

a month ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/92b1623491bf
Introduce a tooltip for the waterfall graph displaying timings. r=rickychien
https://hg.mozilla.org/mozilla-central/rev/92b1623491bf
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

a month ago
Iteration: --- → 55.4 - May 1
Priority: P3 → P1

Comment 19

a month ago
mozreview-review
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
(Assignee)

Comment 20

29 days ago
mozreview-review-reply
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
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
QA Whiteboard: [testday-20170428]

Updated

29 days ago
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
You need to log in before you can comment on or make changes to this bug.