1476549 - Convert frames menu to a doorhanger menu

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Mantaroh Yoshinaga[:mantaroh]]

We can convert the frame menu to doorhanger menu. Furthermore, probably, we can display the favicon by using PlacesUtils.promiseFaviconData()[1]. Devtools server uses this function[2].
And we need to distinguish the type of button which is normal command button(e.g., picker, ruler) and frame button if we will use the MenuButton component.

[1] https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/toolkit/components/places/PlacesUtils.jsm#1513
[2] https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/devtools/server/actors/targets/frame-proxy.js#79-87

Comment 1

[Mantaroh Yoshinaga[:mantaroh]]

I tried using PlacesUtils.promiseFaviconData(). However, it seems to doesn't work if we don't access to target URL as the tab. (e.g., inner frame's favicon will not be stored into Favicon's db.) I think that we need to implement another way to get the favicon.

In this bug, I will convert the frames menu to the doorhanger menu. We need to add the several event handler properties into MenuItem. 

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2a36ee1f9244827e49071a7abe9a7858482d4ca

This wip patch will create the MenuButton instead of the button element. Furthermore, I need to work implementing other features:
 
 * Handling the shortcut of ALT+ALLOW_DOWN to display the popup.
 * Remove unnecessary code from toolbox.js
 * Modifying and adding the tests.

Comment 2

[Mantaroh Yoshinaga[:mantaroh]]

Update:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3707b98a20cd4416b43596fa449cd63e545e6532

Comment 3

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #2)
> Update:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3707b98a20cd4416b43596fa449cd63e545e6532

I need to update the more tests which are related to this change.
Especially, several tests use the showFramesMenu() method.
Updated the patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f119cb454605b74ef332c8a39f93b8bdfa18d27c

Comment 4

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1476549 - Part 1. Rename toolbox.onHightlightFrame to toolbox.onHighlightFrame. r?jdescottes

Comment 5

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1476549 - Part 2. Change the several functions to the local function of ToolboxToolbar. r?jdescottes

This patch will move the global functions of ToolboxToolbar to its local
function. As a result of this changes, each function can refer 'props' from
'this' scope.

Comment 6

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1476549 - Part 3. Make the frame button to MenuButton. r?birtles

This patch makes frame menu to doorhanger menu. ToolboxToolbar will have the map
of target frames in order to make the button. If toolbox.js update this map,
frame menu button will update.

Furthermore, this button should handle the shortcut, so toolbox.js has this
shortcut and click this menu button when receiving this shortcut.

Comment 7

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1476549 - Part 5. Modify the tests which is realted to frames button. r?birtles

Comment 8

[Brian Birtles (:birtles)]

As discussed, I tried the following STR on a debug-opt build:

1. Go to yahoo.com
2. Scroll about one screen's worth
3. Wait about 30 seconds so that all the iframes load
4. Open DevTools
5. Press the frames button

On a debug-opt build it takes 1~2 seconds for the menu to load on a _very_ fast machine. If it's not possible to reproduce that in an opt build it's probably fine but it would be nice to know what is taking so much time. It only happens the first time the menu is opened.

Apart from that, on my screen the number iframes loaded from yahoo.com is more than can fit and the menu is cut off part way. If possible it would be nice to make the menu scroll in that case like the Hamburger -> Library -> Bookmarks menu panel does.

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 9005087 [details]
Bug 1476549 - Part 1. Rename toolbox.onHightlightFrame to toolbox.onHighlightFrame. r?jdescottes

Julian Descottes [:jdescottes][:julian] has approved the revision.

Comment 10

[Julian Descottes [:jdescottes]]

Comment on attachment 9005088 [details]
Bug 1476549 - Part 2. Change the several functions to the local function of ToolboxToolbar. r?jdescottes

Julian Descottes [:jdescottes][:julian] has approved the revision.

Comment 11

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Brian Birtles (:birtles) from comment #8)
> As discussed, I tried the following STR on a debug-opt build:
> 
> 1. Go to yahoo.com
> 2. Scroll about one screen's worth
> 3. Wait about 30 seconds so that all the iframes load
> 4. Open DevTools
> 5. Press the frames button
> 
> On a debug-opt build it takes 1~2 seconds for the menu to load on a _very_
> fast machine. If it's not possible to reproduce that in an opt build it's
> probably fine but it would be nice to know what is taking so much time. It
> only happens the first time the menu is opened.
> 

Thanks, birtles.

I can reproduce this phenomenon even if the target is the opt. The reason is updating frameMap when receiving update-frame event.
If frameMap size is big(e.g, over 200), update component and re-render the component when receiving the update-frame event. The cost of this process is not cheap.  This is sample[1] and the perf-html result of it[2].

[1] https://gist.github.com/mantaroh/17d245095d28eaa35336d611f32ba575
[2] https://perfht.ml/2N0hNYn

Comment 12

[Brian Birtles (:birtles)]

Oh, interesting. (I guess that should be 'frame-update'?)

Debouncing those updates might help, but first it's probably better to check that we're actually doing a minimal update each time. For example, in part 3 we have:

      items.push(MenuItem({
        id: "toolbox-frame-menu-" + index,
        description,
        label,
        checked: frame.id === toolbox.selectedFrameId,
        onFocus: () => toolbox.onHighlightFrame(frame.id),
        onBlur: () => toolbox.highlighterUtils.unhighlight(),
        onMouseOver: () => toolbox.onHighlightFrame(frame.id),
        onMouseOut: () => toolbox.highlighterUtils.unhighlight(),
        onClick: () => toolbox.onSelectFrame(frame.id),
      }));

where all those arrow functions will likely cause React's virtual DOM diffing to update all the existing menu items each time.

We should also add keys for each item like we do for the MeatballMenu[1] so React can match them up.

[1] https://searchfox.org/mozilla-central/rev/05d91d3e02a0780f44599371005591d7988e2809/devtools/client/framework/components/MeatballMenu.js#138

Comment 13

[Mantaroh Yoshinaga[:mantaroh]]

Thank you so much!

(In reply to Brian Birtles (:birtles) from comment #12)
> Oh, interesting. (I guess that should be 'frame-update'?)
>  
> Debouncing those updates might help, but first it's probably better to check
> that we're actually doing a minimal update each time. For example, in part 3
> we have:
> 
>       items.push(MenuItem({
>         id: "toolbox-frame-menu-" + index,
>         description,
>         label,
>         checked: frame.id === toolbox.selectedFrameId,
>         onFocus: () => toolbox.onHighlightFrame(frame.id),
>         onBlur: () => toolbox.highlighterUtils.unhighlight(),
>         onMouseOver: () => toolbox.onHighlightFrame(frame.id),
>         onMouseOut: () => toolbox.highlighterUtils.unhighlight(),
>         onClick: () => toolbox.onSelectFrame(frame.id),
>       }));
> 
> where all those arrow functions will likely cause React's virtual DOM
> diffing to update all the existing menu items each time.
> 
> We should also add keys for each item like we do for the MeatballMenu[1] so
> React can match them up.
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 05d91d3e02a0780f44599371005591d7988e2809/devtools/client/framework/
> components/MeatballMenu.js#138

I think that the React will check the difference of component even if we specified the key property. [1] The result looks to reduce the ToolboxToolbar and MenuItem process a little bit.

I think that we might prevent this performance problem by using shouldUpdateComponent which returning true only if self-property is updated. The result of this change seems to reduce the render time of MenuItem dramatically.[2]
(The time of calling render function is 45ms after changing this. Before modification is 1156ms. )

[1] https://perfht.ml/2NEsbSY
[2] https://perfht.ml/2LJsvxP

Comment 14

[Mantaroh Yoshinaga[:mantaroh]]

Oh, chevron menu don't have the key property, I'll add this modification to part 5 patch.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a40c666b495caedb8e36230d76669a64e5c1083

Comment 15

[Mantaroh Yoshinaga[:mantaroh]]

I decided that Toolbar will make the MenuList when clicking the button since updating the component when receiving the frame-update will be high cost.

WIP patch:
https://hg.mozilla.org/try/rev/acf682a3da1a4bc4b1410e28d2e02c402650fdb7

This behavior will be the same to current frames button. i.e., The popup doesn't display the new frames after the popup is displayed.

Comment 16

[Florens Verschelde :fvsch]

Hi :mantaroh. In bug 1488012 I tried to update the frames icon with a new design, and a feature I’m considering is marking the button as “checked” when the currently targetted frame is a subdocument. This should improve the UX of this tool by providing visual feedback.

I’ll probably wait for your work to land first.
I haven’t looked at your patch yet, but do you know if with a doorhanger menu we get [aria-expanded="true|false"] on the button? That would be helpful to separate the “expanded” state (menu is open) from the “active” state (feature is considered active). Related UX issue: https://github.com/devtools-html/ux/issues/25

Comment 17

[Brian Birtles (:birtles)]

(In reply to Florens Verschelde from comment #16)
> I haven’t looked at your patch yet, but do you know if with a doorhanger
> menu we get [aria-expanded="true|false"] on the button? That would be
> helpful to separate the “expanded” state (menu is open) from the “active”
> state (feature is considered active).

Yes, it will use the MenuButton component which sets aria-expanded:

  https://searchfox.org/mozilla-central/rev/c3fef66a5b211ea8038c1c132706d02db408093a/devtools/client/shared/components/menu/MenuButton.js#281-289

Comment 18

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1476549 - Part 3. Support the children of function type on the MenuButton. r?birtles

This patch will:
 - Call the children as function when rendering if MenuButton's children property is the function.
 - Add the callback which is called when the focused item changed.

Comment 19

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1476549 - Part 4. Make the frame button to MenuButton. r?birtles

Comment 20

[Brian Birtles (:birtles)]

Comment on attachment 9010512 [details]
Bug 1476549 - Part 3. Support the children of function type on the MenuButton. r?birtles

Brian Birtles (:birtles) has approved the revision.

Comment 21

[Brian Birtles (:birtles)]

Comment on attachment 9005090 [details]
Bug 1476549 - Part 5. Modify the tests which is realted to frames button. r?birtles

Brian Birtles (:birtles) has approved the revision.

Comment 22

[Brian Birtles (:birtles)]

Comment on attachment 9010513 [details]
Bug 1476549 - Part 4. Make the frame button to MenuButton. r?birtles

Brian Birtles (:birtles) has approved the revision.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/00691a3d87ef
Part 1. Rename toolbox.onHightlightFrame to toolbox.onHighlightFrame. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/818745407621
Part 2. Change the several functions to the local function of ToolboxToolbar. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/4309a1d872d1
Part 3. Support the children of function type on the MenuButton. r=birtles
https://hg.mozilla.org/integration/autoland/rev/c60ae6c436fb
Part 4. Make the frame button to MenuButton. r=birtles
https://hg.mozilla.org/integration/autoland/rev/1a90b5aa838c
Part 5. Modify the tests which is realted to frames button. r=birtles

Comment 24

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/00691a3d87ef
https://hg.mozilla.org/mozilla-central/rev/818745407621
https://hg.mozilla.org/mozilla-central/rev/4309a1d872d1
https://hg.mozilla.org/mozilla-central/rev/c60ae6c436fb
https://hg.mozilla.org/mozilla-central/rev/1a90b5aa838c