1461522 - Convert meatball menu to a dropdown panel

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Spinning this off for the unfinished work from bug 1455443.

This should allow us to use styling like that bookmarks sidebar panel which has menu items that have *both* checkboxes and icons. It should also allow us to make this menu more consistent with the hamburger menu and less constrained by platform menu conventions.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: WIP patch

I started hacking on this and finally got the arrow style looking right (at least on my machine). Pretty much everything else is broken.

Next steps:

* For the new "menu" type of tooltip, make it prefer to stay inside the window
  where possible like the hamburger menu does (e.g. if activated from a button
  near the right edge of the screen, hang to the left).
* Make the menu hang down (either by setting the preferred height or making
  HTMLTooltip calculate it).
* Get rid of the massive filler div in HTMLTooltip that obscures mouse clicks
  everywhere above the menu.
* Highlight styles for menu items
* Styles for accelerator keys
* Checkboxes
* Icons
* RTL support
* Proper menu toggling
* Better support of no auto hide feature
* Keyboard handling
  - Esc to close
  - Up / down to change highlight
  - Enter to activate menu item
  - Enter to activate menu
* Light / dark theme support
* Zoom handling
* Clean up so this can be more easily used in other components
  (e.g. rename HTMLMenu to HTMLMenuPanel and add an HTMLMenu wrapper component?)
* Clean up CSS so that we fetch ARROW_WIDTH etc. from computed CSS variables and
  don't need to keep JS and CSS in sync manually?

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Oh, and for my own reference, here is the codepen I used to work out the arrow sizing calculations: https://codepen.io/birtles/pen/ZodeKm

Comment 3

[Victoria Wang [:victoria]]

Great to see this started on! Do you still need a mockup, or is it straightforward enough if you borrow from Firefox's tooltip styling? (e.g https://design.firefox.com/photon/components/doorhangers.html)

I like the idea you mentioned in the other bug of bringing back the split console icon. We could also use the old gear icon for settings.

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Oh, that's a helpful link. I just spent the morning making the panel so that, in LTR mode, it always hangs to the right unless that would make it overflow the window (in which case it hangs to the left). According to that doc, however, I should look at whether or not the button in left or right of center.

Specifically it says, "Doorhangers opening on the right side of the view show the directional arrow on the right."

However, for the chevron button I wonder if that's a bit weird. The chevron button will move from left to right depending on the window width, tools enabled etc. Would it be odd that sometimes it hangs to the right and sometimes to the left?

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

That said, making the direction of the hang depend on the position of the anchor would mean a lot less RTL-specific code.

(There are a few things in that doc that seem a little inaccurate. The arrow margin of 16px seems to big -- seems to be 10px for the hamburger menu and bookmarks menu. Likewise, we definitely have no rounding on Linux for those menus. And the arrow seems to be 10x20 as best I can see, not 9x18.)

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I've made this menu follow the guidelines so that for the new menu type it always follows whether it is anchored to a button on the left / right of the document while for existing tooltips it follows the inline direction. That should fix existing tooltips to be more RTL-friendly.

I'm currently working on the vertical positioning. The current HTMLTooltip setup uses a large flexbox with a transparent filler element to get the correct vertical position. Presumably it was trying to accommodate content reflow (with fixed bottom position) without measuring the content height. However, the downside of this is that everything above the tooltip becomes inaccessible. Even if we're careful to catch mouse clicks and echo them on hover styles don't work.

You can confirm this by opening the inspector in a new profile where the 3-pane tooltip comes up. Try to hover anything directly above the tooltip (e.g. the address bar) and you'll see the cursor doesn't change, hover styles don't apply etc.

So, I think my next step is to redo HTMLTooltip's vertical positioning.

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

For my reference, the relevant discussion about HTMLTooltip vertical sizing is in bug 1284461.

I'm a little uneasy about the default "variable height" behavior. It has the drawback of neutering all content north of the tooltip so if we can't find a better approach to that, it feels like we should make it opt-in. For example, the 3-pane inspector tooltip doesn't need this behavior (we should be able to measure the content size once and stick to it) but it gets it by default.

I started looking at fixing this but I'm curious why we constrain the height before calculating the width. Normally in layout we constrain the width first then calculate the height. At a glance it seems like we could do that here:

1. If the preferredWidth is not set call _measureContainerWidth without first setting the height.
2. Call calculateHorizontalPosition. That will clamp the width the to the available space anyway.
3. If the preferredHeight is not set, call a _measureContainerHeight method to get the content height.
4. Call calculateVerticalPosition.

We'd probably want to add some smarts to ensure that we avoid flushing layout twice by skipping step (3) when possible.

Julian, I suspect I'm overlooking something here, but is there a particular reason we set the height first?

For the meatball menu I actually want to set a specific min-height, e.g. 300px or so. In general I think we want the meatball menu to hang down (and scroll if necessary) but if the toolbox is very short and near the bottom of the screen (as I suspect is often the case), I think we want it to hang up. So I think we want to set the min-height in the CSS, measure the content height, and then decide if there is enough room to hang down or up.

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: WIP patch v2

Comment 9

[Julian Descottes [:jdescottes]]

> I'm a little uneasy about the default "variable height" behavior

I agree that variable height as default behavior is not a great idea. It should only apply to tooltips for which the content can be modified after being displayed (for instance, the event tooltip or the autocomplete popup). Feel free to change this.

> I started looking at fixing this but I'm curious why we constrain the height before calculating the width.

I don't think there was a specific reason for picking one over the other. We added the "auto" width option in Bug 1266456 and since we didn't support an "auto" height (which is what you want here) it seemed logical to first set the height before measuring the dimension that was unspecified. Also there was no "variable height" when this was implemented. 

Your proposal seems fine. The only tooltips to carefully test after changing this logic would be the console/inspector autocomplete and the event details tooltip. All the others specify a hardcoded dimensions and use XUL wrappers so they should always be displayed fine. We might be able to stop specifying hardcoded width/height for some of them after your change as well.

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: WIP patch v3

Vertical positioning should now be mostly working.

Still plenty more to do.

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Screenshot (LTR)

Progress screenshot.

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Screenshot (RTL)

Comment 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: WIP patch v4

Still plenty to do here:

* Styling tweaks
  - Too much space between icon and label?
  - Are we getting the right grey for the icons?
* Icons
  - Fix fuziness in "Separate window" icon
  - Add icon for split console
  - Add icon for "Disable menu auto hide"
* RTL
  - The checkmark appears to be on the wrong side
* Proper menu toggling
  (Not sure what this refers to actually, but I'm pretty sure the toggling is
  broken somehow)
* Keyboard handling
  - Esc to close
  - Up / down / enter
  - Enter to activate menu
* Light/dark theme handling
* Zoom handling
* Better handling of "No auto hide menu popup" feature
* Get it to relocate to other windows (i.e. support "Separate window")
* Accessibility -- Check we are using the roles correctly
* Clean up so this can be more easily used in other components
  (e.g. rename HTMLMenu to HTMLMenuPanel and add an HTMLMenu wrapper component?)
* Clean up CSS so that we fetch ARROW_WIDTH etc. from computed CSS variables and
  don't need to keep JS and CSS in sync manually?
* Review all the call sites of HTMLTooltip and confirm I haven't broken them!

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

A couple more todos:

* Make the meatball stay highlighted while the menu is expanded (and darker than the hover state)
  (Probably involves setting the appropriate aria roles for menu open.)
* Hide the tooltip from the meatball button while the menu is expanded

Comment 15

[Victoria Wang [:victoria]]

These screenshots are looking great! Sorry to see that so much has to be done from scratch, including untangling the discrepancies in the design system. It will be great to reuse this work for all the custom DevTools menus.

Comment 16

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Screenshot

I updated the icons to fill the 16x16 box and at the same time I tried updating the styling to match the Photon styling (2px primary border, 3px/1px radius etc.). I'd be interested to hear what you think.

Comment 17

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Screenshot (browser console)

I also added an icon for the disable pop-up autohide item.

Comment 18

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Screenshot (dark theme)

The dark theme should mostly work too although I suspect it will need some tweaking (do the labels need to be lighter?).

Comment 19

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: WIP patch v5

RTL should be fixed in this too.

Comment 20

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: WIP patch v6

Updated to include Mac styling.

(I'm going to obsolete this in a second. I'm just uploading as a backup.)

Comment 21

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: WIP patch v7

This redoes the menu handling to use React components for the menu pieces since it sounds like the stuff we're going to put in these menus are going to get more and more complex (and because it should make it easier to do the key/state handling).

Comment 22

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: WIP patch v8

Fixed menu toggling somewhat and incorporated changes to the menu options.

Comment 23

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: WIP patch v9

A few updates:

* Rebased and now includes extra docking option
* Correctly updates the tooltip when redocking to a new window
* Doesn't consume outside clicks
* Correct highlighting when the menu is open
* Keyboard handling for activating/de-activating the menu

Still to do:

* Keyboard handling within the menu itself
  - Up / down navigation
  - Enter to activate menu item
* Tweak activation of menu by keyboard
  - When activated by keyboard only, call focus() immediately on the menu?
* Zoom handling
* Fix panel resizing so it works when items are added/removed
* Add more spacing between arrow and edge when the corners are rounded (Mac)
* Hide the tooltip from the meatball button while the menu is expanded
* Try to fetch ARROW_WIDTH etc. from computed CSS variables instead of mirroring values in JS
* Review all the call sites of HTMLTooltip and confirm I haven't broken them!

Comment 24

[Victoria Wang [:victoria]]

Re: Dark mode, I realized the doorhangers in Firefox's dark theme are Grey 60 background with Grey 10 text - would probably be good to match them.

It would be great to match the smaller font size and subtler colors of the shortcuts in the Firefox hamburger menu as well - sorry to add more to the todo list :D

Comment 25

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Not at all. I think a lot of that is platform-specific, however. For example, on Linux the font size is the same for the shortcuts in the hamburger menu. I'll prepare a Mac screenshot once I fix the spacing from the arrow when we have rounded corners.

Comment 26

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: WIP patch v10

Rather than having <li> elements with click handlers for menu items, I thought it was probably better from an accessibility point of view to make them <button>s. That way I don't need to add special handling for "Enter" and "Space" to make them activate the menu items.

Now, the whole row needs to be clickable so I was unsure whether to:

a) Have the <button> occupy the full space and rely on it to catch all the clicks.
b) Have the <button> occupy just part of the <li> and then catch clicks on the background and manually call the onClick handler in that case.

I've implemented (a) but it's a little more complex and I suspect I might want to go back to doing (b) at some point. I've probably regressed the layout somewhere along the way too.

All the key handling should now work though including:

* Arrow up and down
* Home / End to jump to the start / end of the list
* Activating the menu using the keyboard pre-selects the first item in the menu

(As per the WAI best practices: https://www.w3.org/TR/wai-aria-practices-1.1/#menu)

Comment 27

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: WIP patch v11

This should maintain a fixed offset even when the border-radius differs.

(I spent most of the time fixing zooming however. It turns out we shouldn't use the value of zoom read from prefs since it's the "ideal" value. We should use the actual zoom value in use. Looks like both HTMLTooltip.js and menu.js have this bug though it probably matters most on the bottom right of large high-dpi screens.)

Comment 28

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I was going to prepare some Mac screenshots but I seemed to have clobbered my Mac build and its 5pm on Friday here so I'll just drop this try link in case anyone wants to see how Mac looks:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e2ceb9b43abd2edb54e733e86780924d7e3c9d6

Comment 29

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: WIP patch v12

This should fix a few Mac-specific issues and redo the dark theme to match the hamburger menu.

Comment 30

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Still to do:

* Fix panel resizing so it works when items are added/removed
  - I have a plan for doing this but it's pretty invasive. I thought about using mutation observers which would be nice and
    encapsulated, but I'm afraid it would too easily lead to recursive behavior and unnecessary updates.
* Address a few tidy ups.
* Fix a number of tests. Some of them need to be all but completely rewritten :/
* Review all the call sites of HTMLTooltip and confirm I haven't broken them.

Comment 31

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Screenshot (dark theme, Mac)

Comment 32

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I've fixed the resizing locally. Victoria, if you want to try this out, here is a build that is mostly complete:

https://queue.taskcluster.net/v1/task/CwnHBGAMSHmmaZBPenuXbw/runs/0/artifacts/public/build/target.dmg

Comment 33

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: WIP patch v13

Adds content resize handling plus fixes a few eslint error from the last try run.

Comment 34

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Hopefully most of the test failures should be fixed now (famous last words):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=22929bd20cb627282ef5c14fea2ffdc1590a2095

Comment 35

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: WIP patch v14

Should be pretty much complete now. Just need to split it up into pieces, check I didn't break anything (which I probably did), and possibly add more tests.

Comment 36

[Victoria Wang [:victoria]]

Thanks for the build link - I just checked this out and it's looking great!! The Photon styling is especially an improvement in dark mode. 

One thing I noticed is the light mode drop shadow seems really intense (as do most of Firefox's doorhangers' shadows) when viewing light-colored pages. The bookmarks and Pocket doorhangers seem to have a different, lighter drop shadow that seems much nicer to my eye. I'm going to ask the UX channel about this.

Comment 37

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Victoria Wang [:victoria] from comment #36)
> One thing I noticed is the light mode drop shadow seems really intense (as
> do most of Firefox's doorhangers' shadows) when viewing light-colored pages.
> The bookmarks and Pocket doorhangers seem to have a different, lighter drop
> shadow that seems much nicer to my eye. I'm going to ask the UX channel
> about this.

Oh, yeah, I'm pretty sure that's a Mac issue--I think it comes about because we end up combining our shadow with a shadow added by the system. I had thought I fixed it (and made it more like the bookmarks doorhanger) but I probably regressed it when I reworked the panels.

Comment 38

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1461522 - Make HTMLTooltip position the arrow correctly in RTL mode;

Currently the arrow offset is not correctly positioned in RTL mode as evidenced
by the test case included in this patch (which fails without the code changes
included in this patch).

This patch logicalizes the calculations for positioning the tooltip which fixes
this and other issues when using RTL mode and will make it easier to introduce
a new "doorhanger" type tooltip later in this patch series.

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

Comment 39

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1461522 - Use the currentZoom as opposed to the pref value;

In a couple of places in DevTools we read back the value of the
"devtools.toolbox.zoomValue" pref and take that to be the zoom value. However,
at certain zoom levels these two can differ: the pref value representing the
ideal zoom and currentZoom giving the actual zoom value in use.

This patch makes us use the actual zoom value in the two places where this
occurs.

Unfortunately, we cannot easily adjust the browser_html_tooltip_zoom.js test for
this since it uses a separate XUL document that does not take into account the
pref value---as a result that test directly sets the currentZoom on the separate
doc and hence this problem won't occur.

Instead, this patch adjusts the browser_toolbox_zoom_popup.js test since the
toolbox menu positioning uses the same problematic pattern so we can reproduce
the bug in the browser_toolbox_zoom_popup.js. (This patch fixes both
occurances of this pattern.)

At least locally browser_toolbox_zoom_popup.js passes for me with a zoom of 1.5
but fails with a zoom of 1.4. Similarly in my testing of HTMLTooltip, zoom
values such as 1.2 and 1.4 often show significant misalignment whilst a zoom of
1.3 or 1.5 is fine.

With the code changes in this patch, the test passes with any given zoom factor.

(This patch also incidentally replaced isNaN with the more robust Number.isNaN.)

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

Comment 40

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1461522 - Fix some comments in HTMLTooltip.js;

A number of lines exceed 80 characters. This patch fixes the lines which are
likely to be touched in subsequent patches to make the code changes easier to
identify. It also adds a missing parameter description.

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

Comment 41

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1461522 - Allow { height: "auto" } in HTMLTooltip setContent and make it the default;

The current default value for height of Infinity has the unfortunate side
effect that, when combined with using a XUL wrapper, there will be a large
filler element stretching vertically on one side of the tooltip that effectively
neuters all content beneath it. While this is probably fine for tooltips that
are shown on hover, it is problematic if we want to use this for DevTools menus
because it means the user is unable to click anything above/below the menu so
long as it is open (which can be particularly problematic once we make
HTMLTooltip support the "Disable popup autohide" feature"). Even if we were to
decide that clicks outside the tooltip should be consumed anyway we would still
have the problem that hover styles don't apply in this "dead" region.

As a result, this patch makes the { height: Infinity } behaviour opt-in for
those tooltips that really need it. For most uses, however, a height calculated
when the tooltip is shown should be sufficient (and later in this patch series
we will add a mechanism to HTMLTooltip to explicitly request it recalculate its
size and position in response to content changes).

This patch introduces the { height: "auto" } mechanism and also reverses the
order in which size/position properties are calculated to match the regular
manner in which layout is performed: widths first, then heights.

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

Comment 42

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1461522 - Add doorhanger type to HTMLTooltip;

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

Comment 43

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1461522 - Factor out a focusableSelector utility;

We will use this later when we introduce a MenuList class since it will want to
manage focus for its menu items.

This patch also extends the definition of focusable elements somewhat.

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

Comment 44

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1461522 - Add focus() and focusEnd() methods to HTMLTooltip;

We will use these methods later when we put menu content inside HTMLTooltip in
order to set the initial focus.

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

Comment 45

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1461522 - Add ID and class properties to HTMLTooltip;

In particular, the ID allows us to associate the tooltip with the button that
triggers it using the aria-controls attribute.

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

Comment 46

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1461522 - Honor ui.popup.disable_autohide setting in HTMLTooltip;

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

Comment 47

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1461522 - Add a mechanism to allow updating an HTMLTooltip's size and position;

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

Comment 48

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1461522 - Add Menu components;

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

Comment 49

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1461522 - Persist active styles for a button with an expanded menu;

When we open the menu, the menu button should remain active so long as the menu
is open.

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

Comment 50

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1461522 - Update browser_toolbox_zoom_popup.js to handle doorhanger menus too;

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

Comment 51

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1461522 - Update comments in ToolboxToolbar.js;

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

Comment 52

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1461522 - Switch meatball menu to a doorhanger;

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

Comment 53

[Brian Birtles (:birtles, Feb 26~ parental leave)]

So, I'm expecting this will take quite a while to get through review but I'm traveling next week so I thought I'd get the process started now so hopefully I can follow up as I get time next week.

I few things which could probably be better:

* I think we might want a larger offset between the arrow and the panel edge for MacOS
  (for Linux and Windows it seems right, but Mac is really inconsistent).
* I don't have any tests for the Menu* components since I wasn't sure how we normally test
  React components that depend on XUL like that. Perhaps I should just test the keyboard handling
  by testing the meatball menu directly?
* I matched the behavior of other menus (hamburger menu, bookmark menu) where clicking the menu
  button while the menu is open re-opens it, but I feel like that's not intuitive for the meatball
  menu somehow.
* Vertical margins for the doorhanger menu could probably use more testing.
* Some of the ARROW_WIDTH style values could probably be put in CSS variables and read back in
  through computed style to remove the fragile linkage there.

Many of those things, however, I'd probably prefer to address in follow ups since this is already a large patch queue that is blocking others (not least being QA) and will probably bitrot quickly.

Comment 54

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Popup test file

In order to test I didn't break existing usage of HTMLTooltip I created this test file and ran the following tests:

* 3-panel inspector tooltip [OK]
  - Set devtools.inspector.show-three-pane-tooltip to true and (re-)open
    toolbox
* Event details tooltip in inspector [OK]
  - Load the test file
  - Inspect the big green test element
  - Open the inspector table
  - Click the "event" token next to the div in the markup view
* Image preview tooltip in inspector [OK]
  - Load the test file
  - Inspect the big bear image at the bottom
  - Find the <img> element in the markup view
  - Hover over the src="..." attribute
* Image preview tooltip in rules view [OK]
  - Load the test file
  - Inspect the big bear image at the bottom
  - In the rules view, find the border-image declaration
  - Hover over the url('...') value
* Font preview tooltip in rules view [OK]
  - Inspect the "Check out my fonts" part of the test file
  - In the rules view find the font-family declaration
  - Hover over "Markazi Text" string
* Variable preview tooltip in rules view [OK]
  - Load the test file
  - Inspect the big green test element
  - Open the Rules panel
  - Hover over the "var(--width)" value of the width property
* Color preview tooltip in rules view [OK]
  - Load the test file
  - Inspect the element
  - Open the Rules panel
  - Click the circle next to the "green" value
* Cubic bezier preview tooltip in rules view [OK]
  - Load the test file
  - Inspect the moving red ball
  - In the rules panel find the "animation" declaration
  - Click the circle next to ease-in
* Filter preview tooltip in rules view [OK]
  - Load the test file
  - Inspect the big bear image at the bottom
  - In the rules view, find the filter declaration
  - Click the circle icon
* Network panel requests view [OK]
  - Open the Network panel
  - Find the request with Filename "240"
  - Hover over the "240"
* Autocomplete popup in inspector searchbox [OK]
  - Load the test page
  - In the box that says "Search HTML" type "div" or "aside"

In addition to the above, I could use QA assistance to test the following:

- Repeat above with the toolbox made very small, the panels very narrow, or
  near the different edges of the screen so that the panels are forced to flip
  to a different side.
- Repeat above with the toolbox made vertical
- Repeat above in RTL mode
- Repeat above with dark theme
- Repeat above on each platform
- Repeat above with zoom 1.3
- Repeat above with zoom 1.4
- Repeat above with various combinations of each setting

I've done spot checks of these only.

Also, apparently there is a popup in the inspector markup view (not the searchbox) but I haven't found it yet.

Comment 55

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988401 [details]
Bug 1461522 - Add a mechanism to allow updating an HTMLTooltip's size and position;

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

Comment 56

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988402 [details]
Bug 1461522 - Add Menu components;

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

Comment 57

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988403 [details]
Bug 1461522 - Persist active styles for a button with an expanded menu;

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

Comment 58

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988404 [details]
Bug 1461522 - Update browser_toolbox_zoom_popup.js to handle doorhanger menus too;

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

Comment 59

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988405 [details]
Bug 1461522 - Update comments in ToolboxToolbar.js;

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

Comment 60

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988406 [details]
Bug 1461522 - Switch meatball menu to a doorhanger;

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

Comment 61

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988401 [details]
Bug 1461522 - Add a mechanism to allow updating an HTMLTooltip's size and position;

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

Comment 62

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988402 [details]
Bug 1461522 - Add Menu components;

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

Comment 63

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988403 [details]
Bug 1461522 - Persist active styles for a button with an expanded menu;

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

Comment 64

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988404 [details]
Bug 1461522 - Update browser_toolbox_zoom_popup.js to handle doorhanger menus too;

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

Comment 65

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988405 [details]
Bug 1461522 - Update comments in ToolboxToolbar.js;

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

Comment 66

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988406 [details]
Bug 1461522 - Switch meatball menu to a doorhanger;

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

Comment 67

[Julian Descottes [:jdescottes]]

Comment on attachment 8988392 [details]
Bug 1461522 - Make HTMLTooltip position the arrow correctly in RTL mode;

https://reviewboard.mozilla.org/r/253674/#review260440

Looks really good!

Comment 68

[Julian Descottes [:jdescottes]]

Comment on attachment 8988393 [details]
Bug 1461522 - Use the currentZoom as opposed to the pref value;

https://reviewboard.mozilla.org/r/253676/#review260444

Works for me, thanks for adding such a detailed commit message!

::: commit-message-64d8d:21
(Diff revision 1)
> +At least locally browser_toolbox_zoom_popup.js passes for me with a zoom of 1.5
> +but fails with a zoom of 1.4. Similarly in my testing of HTMLTooltip, zoom
> +values such as 1.2 and 1.4 often show significant misalignment whilst a zoom of
> +1.3 or 1.5 is fine.

I can't get the test to fail locally. 

Looking at zoom-keys.js, the only way for fullZoom and the pref to be desynchronized is if the setter does some kind of rounding/adjustment on its own?

Maybe we should read contViewer.fullZoom when setting the preference value at https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/devtools/client/shared/zoom-keys.js#58 ?

::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:630
(Diff revision 1)
>      return onPanelShown;
>    },
>  
> +  _getZoom: function() {
> +    const zoom = getCurrentZoom(this.xulPanelWrapper);
> +    return !zoom || Number.isNaN(zoom) ? 1.0 : zoom;

This sanitization made sense when reading the value from a preference. Here feel free to drop it.

Since this method would only work when using the XUL wrapper, I'd prefer to either inline it in _showXULWrapperAt, or make sure the method name mentions XUL.

follow-up material: maybe we can have a toolbox::getZoom method that we call from both places? Means we need to pass toolbox to HTMLTooltip rather than just a document, but that should be fine.

Comment 69

[Julian Descottes [:jdescottes]]

Comment on attachment 8988394 [details]
Bug 1461522 - Fix some comments in HTMLTooltip.js;

https://reviewboard.mozilla.org/r/253678/#review260452

DevTools eslint configuration is set at 90 chars at the moment.
Of course 80 chars is still < 90 chars, but I find it annoying when comments don't use the space available.

Let's keep this in while we finish the review queue. We can discuss this later.

Comment 70

[Julian Descottes [:jdescottes]]

Comment on attachment 8988395 [details]
Bug 1461522 - Allow { height: "auto" } in HTMLTooltip setContent and make it the default;

https://reviewboard.mozilla.org/r/253680/#review260456

Works for me, that's great to have more sensible defaults :)

Can you update the event details tooltip to force the infinite height behavior? 
Created at https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/devtools/client/inspector/markup/markup.js#164 
setContent at https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/devtools/client/shared/widgets/tooltip/EventTooltipHelper.js#200
If I remember correctly, this is the only one which was relying on it. To test it, click on "event" elements in the markup view, and expand one of the events displayed (in case the popup doesn't already fill the toolbox of course)

::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:390
(Diff revision 1)
>      // Get viewport size
>      const viewportRect = this._getViewportRect();
>  
> +    // Calculate the horizonal position and width
> +    let preferredWidth;
> +    let measuredHeight; // Record the height too since it might save us from

nit (consistency): could we move the comments on their own line, same indentation as the code

Comment 71

[Julian Descottes [:jdescottes]]

Note that the patch queue makes https://bugzilla.mozilla.org/show_bug.cgi?id=1470103 slightly. I suppose this is linked to the height: "auto" setting.

Comment 72

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Slightly better or worse?

Comment 73

[Julian Descottes [:jdescottes]]

Sorry, slightly worse :) It makes the height of the autocomplete wrong, as well as the width.

Comment 74

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #68)
> Comment on attachment 8988393 [details]
> Bug 1461522 - Use the currentZoom as opposed to the pref value;
> 
> https://reviewboard.mozilla.org/r/253676/#review260444
> 
> Works for me, thanks for adding such a detailed commit message!
> 
> ::: commit-message-64d8d:21
> (Diff revision 1)
> > +At least locally browser_toolbox_zoom_popup.js passes for me with a zoom of 1.5
> > +but fails with a zoom of 1.4. Similarly in my testing of HTMLTooltip, zoom
> > +values such as 1.2 and 1.4 often show significant misalignment whilst a zoom of
> > +1.3 or 1.5 is fine.
> 
> I can't get the test to fail locally. 

Right, I suspect it never fails on Mac, but it definitely fails on Linux and menus appear in the wrong place too.

> Looking at zoom-keys.js, the only way for fullZoom and the pref to be
> desynchronized is if the setter does some kind of rounding/adjustment on its
> own?

Right, it's a rounding issue.

> Maybe we should read contViewer.fullZoom when setting the preference value
> at
> https://searchfox.org/mozilla-central/rev/
> d2966246905102b36ef5221b0e3cbccf7ea15a86/devtools/client/shared/zoom-keys.
> js#58 ?

I don't think so. I think the rounding is a platform-specific issue. We should store and sync the ideal value or else we might get non-stop updating when syncing between platforms which have a mutually exclusive set of representable values.

> ::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:630
> (Diff revision 1)
> >      return onPanelShown;
> >    },
> >  
> > +  _getZoom: function() {
> > +    const zoom = getCurrentZoom(this.xulPanelWrapper);
> > +    return !zoom || Number.isNaN(zoom) ? 1.0 : zoom;
> 
> This sanitization made sense when reading the value from a preference. Here
> feel free to drop it.

Good idea.

> Since this method would only work when using the XUL wrapper, I'd prefer to
> either inline it in _showXULWrapperAt, or make sure the method name mentions
> XUL.

Fixed (by making it inline).

Comment 75

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #69)
> Comment on attachment 8988394 [details]
> Bug 1461522 - Fix some line wrapping in HTMLTooltip.js;
> 
> https://reviewboard.mozilla.org/r/253678/#review260452
> 
> DevTools eslint configuration is set at 90 chars at the moment.
> Of course 80 chars is still < 90 chars, but I find it annoying when comments
> don't use the space available.

Fair enough. Where should I follow up on this? I heard there's some sort of RFC process?

> Let's keep this in while we finish the review queue. We can discuss this
> later.

I've dropped the line wrapping changes and left the content changes in.

Comment 76

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #70)
> Comment on attachment 8988395 [details]
> Bug 1461522 - Allow { height: "auto" } in HTMLTooltip setContent and make it
> the default;
> 
> https://reviewboard.mozilla.org/r/253680/#review260456
> 
> Works for me, that's great to have more sensible defaults :)
> 
> Can you update the event details tooltip to force the infinite height
> behavior? 
> Created at
> https://searchfox.org/mozilla-central/rev/
> d2966246905102b36ef5221b0e3cbccf7ea15a86/devtools/client/inspector/markup/
> markup.js#164 
> setContent at
> https://searchfox.org/mozilla-central/rev/
> d2966246905102b36ef5221b0e3cbccf7ea15a86/devtools/client/shared/widgets/
> tooltip/EventTooltipHelper.js#200
> If I remember correctly, this is the only one which was relying on it. To
> test it, click on "event" elements in the markup view, and expand one of the
> events displayed (in case the popup doesn't already fill the toolbox of
> course)

Ah, I didn't know you could click them! Fixed.

Comment 77

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988392 [details]
Bug 1461522 - Make HTMLTooltip position the arrow correctly in RTL mode;

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

Comment 78

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988393 [details]
Bug 1461522 - Use the currentZoom as opposed to the pref value;

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

Comment 79

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988394 [details]
Bug 1461522 - Fix some comments in HTMLTooltip.js;

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

Comment 80

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988395 [details]
Bug 1461522 - Allow { height: "auto" } in HTMLTooltip setContent and make it the default;

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

Comment 81

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988396 [details]
Bug 1461522 - Add doorhanger type to HTMLTooltip;

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

Comment 82

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988397 [details]
Bug 1461522 - Factor out a focusableSelector utility;

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

Comment 83

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988398 [details]
Bug 1461522 - Add focus() and focusEnd() methods to HTMLTooltip;

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

Comment 84

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988399 [details]
Bug 1461522 - Add ID and class properties to HTMLTooltip;

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

Comment 85

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988400 [details]
Bug 1461522 - Honor ui.popup.disable_autohide setting in HTMLTooltip;

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

Comment 86

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988401 [details]
Bug 1461522 - Add a mechanism to allow updating an HTMLTooltip's size and position;

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

Comment 87

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988402 [details]
Bug 1461522 - Add Menu components;

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

Comment 88

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988403 [details]
Bug 1461522 - Persist active styles for a button with an expanded menu;

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

Comment 89

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988404 [details]
Bug 1461522 - Update browser_toolbox_zoom_popup.js to handle doorhanger menus too;

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

Comment 90

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988405 [details]
Bug 1461522 - Update comments in ToolboxToolbar.js;

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

Comment 91

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988406 [details]
Bug 1461522 - Switch meatball menu to a doorhanger;

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

Comment 92

[Julian Descottes [:jdescottes]]

(In reply to Brian Birtles (:birtles, traveling 29 June - 6 July) from comment #75)
> (In reply to Julian Descottes [:jdescottes][:julian] from comment #69)
> > Comment on attachment 8988394 [details]
> > Bug 1461522 - Fix some line wrapping in HTMLTooltip.js;
> > 
> > https://reviewboard.mozilla.org/r/253678/#review260452
> > 
> > DevTools eslint configuration is set at 90 chars at the moment.
> > Of course 80 chars is still < 90 chars, but I find it annoying when comments
> > don't use the space available.
> 
> Fair enough. Where should I follow up on this? I heard there's some sort of
> RFC process?
> 
> > Let's keep this in while we finish the review queue. We can discuss this
> > later.
> 
> I've dropped the line wrapping changes and left the content changes in.

Cool! For RFCs, you can create an issue on https://github.com/devtools-html/rfcs/ detailing your proposal. The process is pretty lightweight, just describe clearly the change, people will comment and once we have consensus we will follow up with actions.

FWIW, I am not a big fan of this rule making our code inconsistent with the rest of the codebase. But the initial discussion was at https://groups.google.com/forum/#!msg/mozilla.dev.developer-tools/zcWUzmbY2N0/J6iZT_H4LgAJ;context-place=forum/mozilla.dev.developer-tools

Comment 93

[Julian Descottes [:jdescottes]]

Attachment: arrow_floating_over_rounded_corner.png

Comment 94

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #93)
> Created attachment 8989127 [details]
> arrow_floating_over_rounded_corner.png

Oh, I haven't seen that before. I guess I broke something when I rebuilt the patches.

Comment 95

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #92)
> FWIW, I am not a big fan of this rule making our code inconsistent with the
> rest of the codebase. But the initial discussion was at
> https://groups.google.com/forum/#!msg/mozilla.dev.developer-tools/
> zcWUzmbY2N0/J6iZT_H4LgAJ;context-place=forum/mozilla.dev.developer-tools

Looking at that thread, it looks like the intention was to wrap to 80 but let the *eslint rule* be more flexible so that we don't have to go and rework a lot of files just to keep eslint happy. i.e. the Mozilla coding style for JS still applies.

So it sounds like wrapping to 80 is still a good idea, and wrapping existing lines when we touch them is also a good idea. Anyway, I'm not going to push for that in this bug, though.

Comment 96

[Julian Descottes [:jdescottes]]

(In reply to Brian Birtles (:birtles, traveling 29 June - 6 July) from comment #94)
> (In reply to Julian Descottes [:jdescottes][:julian] from comment #93)
> > Created attachment 8989127 [details]
> > arrow_floating_over_rounded_corner.png
> 
> Oh, I haven't seen that before. I guess I broke something when I rebuilt the
> patches.

It's a very special edge case, the attachment is related to a review comment for an ongoing review. Will be clearer when I publish it.

(In reply to Brian Birtles (:birtles, traveling 29 June - 6 July) from comment #95)
> (In reply to Julian Descottes [:jdescottes][:julian] from comment #92)
> > FWIW, I am not a big fan of this rule making our code inconsistent with the
> > rest of the codebase. But the initial discussion was at
> > https://groups.google.com/forum/#!msg/mozilla.dev.developer-tools/
> > zcWUzmbY2N0/J6iZT_H4LgAJ;context-place=forum/mozilla.dev.developer-tools
> 
> Looking at that thread, it looks like the intention was to wrap to 80 but
> let the *eslint rule* be more flexible so that we don't have to go and
> rework a lot of files just to keep eslint happy. i.e. the Mozilla coding
> style for JS still applies.
> 
> So it sounds like wrapping to 80 is still a good idea, and wrapping existing
> lines when we touch them is also a good idea. Anyway, I'm not going to push
> for that in this bug, though.

Let's move the discussion to either Slack or RFC, but to clarify: we had 80 chars enforced by eslint until mid 2016. The thread was about removing the limitation, which was not accepted, the compromise was to extend the limit to 90 instead.

Comment 97

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #96)
> (In reply to Brian Birtles (:birtles, traveling 29 June - 6 July) from
> comment #94)
> > (In reply to Julian Descottes [:jdescottes][:julian] from comment #93)
> > > Created attachment 8989127 [details]
> > > arrow_floating_over_rounded_corner.png
> > 
> > Oh, I haven't seen that before. I guess I broke something when I rebuilt the
> > patches.
> 
> It's a very special edge case, the attachment is related to a review comment
> for an ongoing review. Will be clearer when I publish it.

Yeah, I can reproduce it. I'm sure I tested that case early in the process though and fixed it, so I must have broken it again.

Comment 98

[Julian Descottes [:jdescottes]]

Comment on attachment 8988396 [details]
Bug 1461522 - Add doorhanger type to HTMLTooltip;

https://reviewboard.mozilla.org/r/253682/#review260944

Overall this looks very good! A few nits, comments and questions.

::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:60
(Diff revision 2)
> +  "doorhanger": 9,
>  };
>  
>  const EXTRA_BORDER = {
>    "normal": 0,
>    "arrow": 3,

Should we add `"doorhanger": 0` here?

::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:189
(Diff revision 2)
> +  //
> +  // So for those we need to check if the anchor is more right or left.
> +  let hangDirection;
> +  if (type === TYPE.DOORHANGER) {
> +    const anchorCenter = anchorRect.left + anchorRect.width / 2;
> +    const viewCenter = windowRect.left + windowRect.width / 2;

I am not sure about using this new "windowRect" to compute the hangDirection. 

Here we make the assumption that the toolbox window is always the reference element (ie "the view"  from the UX guidelines). Would that still be true if we had a doorhanger inside an inspector sidepanel for instance (eg animation inspector)? 

An alternative would be to let the caller pass the desired hangDirection for doorhanger tooltips.

::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:213
(Diff revision 2)
>    tooltipStart = Math.min(tooltipStart, viewportRect.width - tooltipWidth);
>    tooltipStart = Math.max(0, tooltipStart);
>  
>    // Calculate arrow start (tooltip's start might be updated)
> -  const arrowWidth = type === TYPE.ARROW ? ARROW_WIDTH : 0;
> +  const arrowWidth =
> +    (type === TYPE.ARROW) | (type === TYPE.DOORHANGER) ? ARROW_WIDTH[type] : 0;

Should that be || rather than | ?

Also you added values in ARROW_WIDTH for all types, so can we just do `const arrowWidth = ARROW_WIDTH[type]` ?

::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:233
(Diff revision 2)
>      // Make sure the arrow remains in the tooltip container.
>      arrowStart = Math.min(arrowStart, tooltipWidth - arrowWidth);
>      arrowStart = Math.max(arrowStart, 0);

We should take borderRadius into account here, otherwise some edge cases can make the arrow "float" over the rounded corner on OSX (added a screenshot)

::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:431
(Diff revision 2)
>      let anchorRect = getRelativeRect(anchor, this.doc);
>      if (this.useXulWrapper) {
>        anchorRect = this._convertToScreenRect(anchorRect);
>      }
>  
> -    // Get viewport size
> +    // Get document size

This comment is a bit superfluous and could probably be removed.

::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:458
(Diff revision 2)
> +      anchorWin
> +        .getComputedStyle(anchor)

Maybe we can use a single anchorWin.getComputedStyle(anchor) for isRtl and for borderRadius?

::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:460
(Diff revision 2)
>      const isRtl = anchorWin.getComputedStyle(anchor).direction === "rtl";
> +
> +    let borderRadius = parseFloat(
> +      anchorWin
> +        .getComputedStyle(anchor)
> +        .getPropertyValue("--theme-arrowpanel-border-radius")

I think borderRadius should always be 0 for ARROW and NORMAL types. ARROW type has a platform agnostic 3px border radius, but it's already "handled" via the ARROW_OFFSET.

::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:559
(Diff revision 2)
> -   * Calculate the rect of the viewport that limits the tooltip dimensions. When using a
> +   * Calculate the following boundary rectangles:
> -   * XUL panel wrapper, the viewport will be able to use the whole screen (excluding space
> -   * reserved by the OS for toolbars etc.). Otherwise, the viewport is limited to the
> -   * tooltip's document.
>     *
> -   * @return {Object} DOMRect-like object with the Number properties: top, right, bottom,
> +   * - Viewport rect: This is the region that the tooltip dimensions. When

nit: `that the tooltip dimensions` -> `that limits the tooltip dimensions` or similar

::: devtools/client/themes/tooltips.css:268
(Diff revision 2)
>  }
>  
> +/* Tooltip : doorhanger style */
> +
> +:root {
> +  --theme-arrowpanel-border-radius: 0px;

nit: 0px -> 0

::: devtools/client/themes/tooltips.css:268
(Diff revision 2)
>  }
>  
> +/* Tooltip : doorhanger style */
> +
> +:root {
> +  --theme-arrowpanel-border-radius: 0px;

As a general note: why use the term arrowpanel rather than doorhanger here?

::: devtools/client/themes/tooltips.css:392
(Diff revision 2)
> +  /* Override the display: -moz-box declaration in minimal-xul.css
> +   * or else menu items won't stack. */
> +  display: block;
> +}
> +
> +.tooltip-container[type="doorhanger"] li > .command {

Up to you, but maybe it could be nice to come up with a dedicated classname for the `li` of the doorhanger menu? 

And I wonder if all the styles really specific to the content could be moved to a dedicated CSS file? That's what we do for the various editors in the inspector, they all have their dedicated CSS file (eg devtools/client/shared/widgets/filter-widget.css) which are imported at the beginning of tooltips.css.

Of course this is a good candidate for a follow-up

::: devtools/client/themes/variables.css:35
(Diff revision 2)
>    --theme-toolbar-background-hover: rgba(221, 225, 228, 0.66);
>    --theme-toolbar-background-alt: #f5f5f5;
>    --theme-toolbar-hover: var(--grey-20);
>    --theme-toolbar-hover-active: var(--grey-20);
>  
> +  /* Doorhangers */

I am always unsure about where to put CSS variables in DevTools, those might be more appropriate in tooltips.css? 

I would in any case have them after the more generic variables, eg after "--theme-tooltip-shadow".

::: devtools/client/themes/variables.css:37
(Diff revision 2)
>    --theme-toolbar-hover: var(--grey-20);
>    --theme-toolbar-hover-active: var(--grey-20);
>  
> +  /* Doorhangers */
> +  --theme-arrowpanel-background: white;
> +  --theme-arrowpanel-color: -moz-fieldText;

Can we use an existing variable instead?

::: devtools/client/themes/variables.css:42
(Diff revision 2)
> +  --theme-arrowpanel-color: -moz-fieldText;
> +  --theme-arrowpanel-border-color: var(--grey-90-a20);
> +  --theme-arrowpanel-separator: var(--grey-90-a20);
> +  --theme-arrowpanel-dimmed: hsla(0,0%,80%,.3);
> +  --theme-arrowpanel-dimmed-further: hsla(0,0%,80%,.45);
> +  --theme-arrowpanel-disabled-color: GrayText;

Any reason to use GrayText over an existing "disabled" DevTools variable eg --theme-body-color-inactive?

::: devtools/client/themes/variables.css:113
(Diff revision 2)
>  
>    --theme-codemirror-gutter-background: #f4f4f4;
>    --theme-messageCloseButtonFilter: invert(0);
>  }
>  
> +:root[platform="mac"].theme-light {

Is there any relevant comment we could add to understand why we need different colors for OSX?

::: devtools/client/themes/variables.css:115
(Diff revision 2)
>    --theme-messageCloseButtonFilter: invert(0);
>  }
>  
> +:root[platform="mac"].theme-light {
> +  --theme-arrowpanel-color: rgb(26,26,26);
> +  --theme-arrowpanel-border-color: hsl(210,4%,10%,.05);

ah I didn't know that hsl was an alias for hsla (or vice versa). Might be more consistent to use hsla() here for consistency.

Comment 99

[Julian Descottes [:jdescottes]]

Comment on attachment 8988397 [details]
Bug 1461522 - Factor out a focusableSelector utility;

https://reviewboard.mozilla.org/r/253684/#review261008

Works for me. 
I could see this file move to devtools/shared/css/focus.js (or focusable-selector.js). 
But both folders are a bit messy at the moment so no strong preference.

Comment 100

[Julian Descottes [:jdescottes]]

Comment on attachment 8988397 [details]
Bug 1461522 - Factor out a focusableSelector utility;

https://reviewboard.mozilla.org/r/253684/#review261010

One additional note about focus and HTMLTooltip: the current implementation came from the fact that we wanted something free of Chrome privileged APIs (devtools-html).
Maybe it is time to revisit that and use more powerful APIs, eg nsIFocusManager::moveFocus

https://bugzilla.mozilla.org/show_bug.cgi?id=1266450#c53

Comment 101

[Julian Descottes [:jdescottes]]

Comment on attachment 8988398 [details]
Bug 1461522 - Add focus() and focusEnd() methods to HTMLTooltip;

https://reviewboard.mozilla.org/r/253686/#review261012

Thanks!

::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:549
(Diff revision 2)
>      // Keep a pointer on the focused element to refocus it when hiding the tooltip.
>      this._focusedElement = this.doc.activeElement;
>  
>      this.doc.defaultView.clearTimeout(this.attachEventsTimer);
>      this.attachEventsTimer = this.doc.defaultView.setTimeout(() => {
> -      this._maybeFocusTooltip();
> +      if (this.autofocus) {

This "autofocus" feature was initially meant for the Debugger variable view tooltip which was never migrated to HTML. I wonder if we should remove it.

Comment 102

[Julian Descottes [:jdescottes]]

Comment on attachment 8988398 [details]
Bug 1461522 - Add focus() and focusEnd() methods to HTMLTooltip;

https://reviewboard.mozilla.org/r/253686/#review261014

Comment 103

[Julian Descottes [:jdescottes]]

Comment on attachment 8988399 [details]
Bug 1461522 - Add ID and class properties to HTMLTooltip;

https://reviewboard.mozilla.org/r/253688/#review261016

::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:289
(Diff revision 2)
> + *        - {String} className
> + *          Additional classes to add to the tooltip container element.

Maybe mention that we expect the String to be a space-separated list of classes?

I don't think I have seen this used at any point in the series, is this intended for an upcoming bug?

Comment 104

[Julian Descottes [:jdescottes]]

Comment on attachment 8988400 [details]
Bug 1461522 - Honor ui.popup.disable_autohide setting in HTMLTooltip;

https://reviewboard.mozilla.org/r/253690/#review261032

::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:744
(Diff revision 2)
>  
> +    // If the disable autohide setting is in effect, ignore.
> +    //
> +    // REVIEWER: Should we setup a pref observer for this?
> +    // Or use the prefs front? (devtools/shared/fronts/preference)
> +    // Or the PrefsHelper? (devtools/client/shared/prefs)

Preferences Front is used to interact with the preferences of the debuggee (interesting in case of remote debugging). That's usually true for all fronts, they are client objects that allow to interact with actors living on the debuggee. 

Here we only care about the local value of the preference, so it's either client/shared/prefs or Services.prefs. 

I don't have a strong preference since the codebase is inconsistent. We recently went through a RFC to unify our usage of telemetry, which seems similar to this situation: https://github.com/devtools-html/rfcs/issues/47 I haven't reviewed in details the added value of shared/prefs over using Services.prefs directly but feel free to bring it up to the team.

I don't think an observer is needed here? We don't need to react to the preference change. The only valid case would be to cache the pref value on change and avoid calling Services.prefs on every click, but I doubt this will be on hot path.

Comment 105

[Julian Descottes [:jdescottes]]

Comment on attachment 8988401 [details]
Bug 1461522 - Add a mechanism to allow updating an HTMLTooltip's size and position;

https://reviewboard.mozilla.org/r/253692/#review261044

::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:483
(Diff revision 4)
> +      this.container.style.left = left + "px";
> +      this.container.style.top = top + "px";
> +    }
> +  },
> +
> +  _resizeContent(anchor, {position, x = 0, y = 0} = {}) {

This is in theory doing more than resizing (and it's resizing the container rather than the content), but I can't really find a much better name... update? adaptToContent?

Comment 106

[Julian Descottes [:jdescottes]]

Comment on attachment 8988402 [details]
Bug 1461522 - Add Menu components;

https://reviewboard.mozilla.org/r/253694/#review261048

Thanks for the patch! There is clear overlap between this new menu and the one provided by client/framework./menu.js
We should clarify this either with better comments or different names to explain when and why you should pick one over the other.
Could you file a follow up for this?

::: devtools/client/shared/components/menu/MenuButton.js:38
(Diff revision 4)
> +      // The offset of the menu from the anchor element.
> +      // Defaults to -5.
> +      menuOffset: PropTypes.number.isRequired,
> +
> +      // The menu content.
> +      children: PropTypes.func.isRequired,

nit: PropTypes.any?

::: devtools/client/shared/components/menu/MenuButton.js:117
(Diff revision 4)
> +  resetTooltip() {
> +    if (!this.tooltip) {
> +      return;
> +    }
> +
> +    this.tooltip.destroy();

If the tooltip is visible, destroy() will call hide() which will emit a "hidden" event asynchronously. So we might remove the onHidden listener before the event is fired. Should we update the state to set expanded to false manually and synchronously here?

::: devtools/client/shared/components/menu/MenuButton.js:137
(Diff revision 4)
> +      position: this.props.menuPosition,
> +      y: this.props.menuOffset,
> +    });
> +  }
> +
> +  async closeMenu() {

nit: naming: hideMenu to mirror showMenu?

::: devtools/client/shared/components/menu/MenuButton.js:153
(Diff revision 4)
> +
> +  async toggleMenu(anchor) {
> +    return this.state.expanded ? this.closeMenu() : this.showMenu(anchor);
> +  }
> +
> +  resizeContent() {

Maybe add a comment to explain that this an exposed API and not used internally.

::: devtools/client/shared/components/menu/MenuButton.js:175
(Diff revision 4)
> +      if (this.props.onClick) {
> +        this.props.onClick(e);
> +      }
> +
> +      if (!e.defaultPrevented) {
> +        const wasKeyboardEvent = e.screenX === 0 && e.screenY === 0;

it's a bit sad that this is our only way to identify a keyboard-triggered click event.

::: devtools/client/shared/components/menu/MenuButton.js:208
(Diff revision 4)
> +        e.preventDefault();
> +        break;
> +
> +      case "Tab":
> +      case "ArrowDown":
> +        if (isButtonFocussed && this.tooltip && this.tooltip.focus()) {

Could we move this.tooltip.focus() inside the if block? I know this will add some nesting but I feel like the current version hides the fact that we are focusing.

Same comment for focusEnd

::: devtools/client/shared/components/menu/MenuButton.js:222
(Diff revision 4)
> +        break;
> +    }
> +  }
> +
> +  render() {
> +    assert(this.tooltip);

We rarely use assert() in client code, so just making sure you left this intentionally?

::: devtools/client/shared/components/menu/MenuButton.js:224
(Diff revision 4)
> +    const menu = ReactDOM.createPortal(
> +      this.props.children,
> +      this.tooltip.panel
> +    );

Here we are completely bypassing the call to setContent for this tooltip. We should make this a bit more explicit with a comment. 

And maybe file a follow up to make this the only supported behavior of the tooltip: every caller would set the content via tooltip.panel.appendChild and would set { width, height } either via another method or via additional parameters in show()?

::: devtools/client/shared/components/menu/MenuButton.js:234
(Diff revision 4)
> +    const buttonProps = {
> +      ...this.props,
> +      onClick: this.onClick,
> +      "aria-expanded": this.state.expanded,
> +      "aria-haspopup": "menu",
> +      ref: this.setButtonRef,

Why use a function for `ref` here rather than setting ref: "buttonRef" and accessing it via this.refs.buttonRef ?

Same question applies to the other 2 components

::: devtools/client/shared/components/menu/MenuList.js:8
(Diff revision 4)
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* eslint-env browser */
> +"use strict";
> +
> +// An list of menu items.

nit: a list

::: devtools/client/shared/components/menu/MenuList.js:52
(Diff revision 4)
> +    );
> +
> +    switch (e.key) {
> +      case "ArrowUp":
> +      case "ArrowDown":
> +        if (focusIsInList()) {

we could bail out early if !focusIsInList() rather than having it in each `case`?

Comment 107

[Julian Descottes [:jdescottes]]

Comment on attachment 8988403 [details]
Bug 1461522 - Persist active styles for a button with an expanded menu;

https://reviewboard.mozilla.org/r/253696/#review261092

Comment 108

[Julian Descottes [:jdescottes]]

Comment on attachment 8988405 [details]
Bug 1461522 - Update comments in ToolboxToolbar.js;

https://reviewboard.mozilla.org/r/253700/#review261094

::: devtools/client/framework/components/ToolboxToolbar.js:47
(Diff revision 4)
>        // Current docking type. Typically one of the position values in
>        // |hostTypes| but this is not always the case (e.g. when it is "custom").
>        currentHostType: PropTypes.string,
> +      // Are docking options enabled? They are not enabled in certain situations
> +      // like when they are in the WebIDE.
> +      areDockOptionsEnabled: PropTypes.boolean,

PropTypes.boolean -> PropTypes.bool

Comment 109

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #98)
> ::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:60
> (Diff revision 2)
> > +  "doorhanger": 9,
> >  };
> >  
> >  const EXTRA_BORDER = {
> >    "normal": 0,
> >    "arrow": 3,
> 
> Should we add `"doorhanger": 0` here?

Definitely. I think I broke this when I rebased it.

> ::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:189
> (Diff revision 2)
> > +  //
> > +  // So for those we need to check if the anchor is more right or left.
> > +  let hangDirection;
> > +  if (type === TYPE.DOORHANGER) {
> > +    const anchorCenter = anchorRect.left + anchorRect.width / 2;
> > +    const viewCenter = windowRect.left + windowRect.width / 2;
> 
> I am not sure about using this new "windowRect" to compute the
> hangDirection. 
> 
> Here we make the assumption that the toolbox window is always the reference
> element (ie "the view"  from the UX guidelines). Would that still be true if
> we had a doorhanger inside an inspector sidepanel for instance (eg animation
> inspector)? 
> 
> An alternative would be to let the caller pass the desired hangDirection for
> doorhanger tooltips.

The doorhanger position needs to depend on where the anchor is in relation to the window. So, for example, if we use this for the chevron button the hang direction will depend on how far along the chevron is positioned, which side it is docked etc. If the toolbox is docked to the left, it should hang to the right etc. So I don't think we can pass this in without putting a lot of burden on each call site. My read of the UX guidelines based on testing how other doorhangers work is that this should be relative to the outermost window (although perhaps I've chosen the wrong window? It seemed right when I tested).

> ::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:213
> (Diff revision 2)
> >    tooltipStart = Math.min(tooltipStart, viewportRect.width - tooltipWidth);
> >    tooltipStart = Math.max(0, tooltipStart);
> >  
> >    // Calculate arrow start (tooltip's start might be updated)
> > -  const arrowWidth = type === TYPE.ARROW ? ARROW_WIDTH : 0;
> > +  const arrowWidth =
> > +    (type === TYPE.ARROW) | (type === TYPE.DOORHANGER) ? ARROW_WIDTH[type] : 0;
> 
> Should that be || rather than | ?

Yes.
 
> Also you added values in ARROW_WIDTH for all types, so can we just do `const
> arrowWidth = ARROW_WIDTH[type]` ?

I have no idea what I was thinking there... (that was something I added at the last moment and I have no idea why).

> ::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:233
> (Diff revision 2)
> >      // Make sure the arrow remains in the tooltip container.
> >      arrowStart = Math.min(arrowStart, tooltipWidth - arrowWidth);
> >      arrowStart = Math.max(arrowStart, 0);
> 
> We should take borderRadius into account here, otherwise some edge cases can
> make the arrow "float" over the rounded corner on OSX (added a screenshot)

Yep, good catch.

> ::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:458
> (Diff revision 2)
> > +      anchorWin
> > +        .getComputedStyle(anchor)
> 
> Maybe we can use a single anchorWin.getComputedStyle(anchor) for isRtl and
> for borderRadius?

From a performance point of view, it doesn't make any difference. getComputedStyle() itself doesn't do the work of flushing style/layout -- accessing its properties does.
 
> ::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:460
> (Diff revision 2)
> >      const isRtl = anchorWin.getComputedStyle(anchor).direction === "rtl";
> > +
> > +    let borderRadius = parseFloat(
> > +      anchorWin
> > +        .getComputedStyle(anchor)
> > +        .getPropertyValue("--theme-arrowpanel-border-radius")
> 
> I think borderRadius should always be 0 for ARROW and NORMAL types. ARROW
> type has a platform agnostic 3px border radius, but it's already "handled"
> via the ARROW_OFFSET.

Yes. Long-term I'd like to get rid of ARROW_OFFSET etc. and get everything from CSS variables in computed style so I was trying to move that direction in this patch. Perhaps that can come later though if you're concerned about perf here.

Comment 110

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #98)
> Comment on attachment 8988396 [details]
> Bug 1461522 - Add doorhanger type to HTMLTooltip;
> ...
> ::: devtools/client/themes/tooltips.css:268
> (Diff revision 2)
> >  }
> >  
> > +/* Tooltip : doorhanger style */
> > +
> > +:root {
> > +  --theme-arrowpanel-border-radius: 0px;
> 
> nit: 0px -> 0

Why is this?

(From a CSS point of view, the unitless 0 is generally considered a mistake that we would like to remove if possible.)

> ::: devtools/client/themes/tooltips.css:268
> (Diff revision 2)
> >  }
> >  
> > +/* Tooltip : doorhanger style */
> > +
> > +:root {
> > +  --theme-arrowpanel-border-radius: 0px;
> 
> As a general note: why use the term arrowpanel rather than doorhanger here?

I was trying to match the terminology elsewhere in Firefox. Is that ok?

> ::: devtools/client/themes/tooltips.css:392
> (Diff revision 2)
> > +  /* Override the display: -moz-box declaration in minimal-xul.css
> > +   * or else menu items won't stack. */
> > +  display: block;
> > +}
> > +
> > +.tooltip-container[type="doorhanger"] li > .command {
> 
> Up to you, but maybe it could be nice to come up with a dedicated classname
> for the `li` of the doorhanger menu? 

Yeah, .menuitem or something like that?

> And I wonder if all the styles really specific to the content could be moved
> to a dedicated CSS file? That's what we do for the various editors in the
> inspector, they all have their dedicated CSS file (eg
> devtools/client/shared/widgets/filter-widget.css) which are imported at the
> beginning of tooltips.css.

Yeah, all these are supposed to be generic menu styles (not specific to the meatball menu). Originally I named the "doorhanger" type the "menu" type and it made sense at that point to include them here but now that we're calling it "doorhanger" it probably makes sense to move these to a separate file.

> Of course this is a good candidate for a follow-up

Filed bug 1472904 for that.

> ::: devtools/client/themes/variables.css:35
> (Diff revision 2)
> >    --theme-toolbar-background-hover: rgba(221, 225, 228, 0.66);
> >    --theme-toolbar-background-alt: #f5f5f5;
> >    --theme-toolbar-hover: var(--grey-20);
> >    --theme-toolbar-hover-active: var(--grey-20);
> >  
> > +  /* Doorhangers */
> 
> I am always unsure about where to put CSS variables in DevTools, those might
> be more appropriate in tooltips.css? 

Yeah, I wasn't sure. I got the impression that we tried to put most of the theme-specific variables in variables.css so it was easier to tweaks themes in one place.

> I would in any case have them after the more generic variables, eg after
> "--theme-tooltip-shadow".

Sure.

> ::: devtools/client/themes/variables.css:37
> (Diff revision 2)
> >    --theme-toolbar-hover: var(--grey-20);
> >    --theme-toolbar-hover-active: var(--grey-20);
> >  
> > +  /* Doorhangers */
> > +  --theme-arrowpanel-background: white;
> > +  --theme-arrowpanel-color: -moz-fieldText;
> 
> Can we use an existing variable instead?

I'm not sure I follow. What's the advantage of that? Doesn't it lead to changes having unforeseen consequences?

> ::: devtools/client/themes/variables.css:42
> (Diff revision 2)
> > +  --theme-arrowpanel-color: -moz-fieldText;
> > +  --theme-arrowpanel-border-color: var(--grey-90-a20);
> > +  --theme-arrowpanel-separator: var(--grey-90-a20);
> > +  --theme-arrowpanel-dimmed: hsla(0,0%,80%,.3);
> > +  --theme-arrowpanel-dimmed-further: hsla(0,0%,80%,.45);
> > +  --theme-arrowpanel-disabled-color: GrayText;
> 
> Any reason to use GrayText over an existing "disabled" DevTools variable eg
> --theme-body-color-inactive?

As before (and also to mirror the usage elsewhere in Firefox). Let me know what you prefer here.

> ::: devtools/client/themes/variables.css:113
> (Diff revision 2)
> >  
> >    --theme-codemirror-gutter-background: #f4f4f4;
> >    --theme-messageCloseButtonFilter: invert(0);
> >  }
> >  
> > +:root[platform="mac"].theme-light {
> 
> Is there any relevant comment we could add to understand why we need
> different colors for OSX?

Yeah, although it's basically just "Because that's what we do elsewhere in Firefox".

> ::: devtools/client/themes/variables.css:115
> (Diff revision 2)
> >    --theme-messageCloseButtonFilter: invert(0);
> >  }
> >  
> > +:root[platform="mac"].theme-light {
> > +  --theme-arrowpanel-color: rgb(26,26,26);
> > +  --theme-arrowpanel-border-color: hsl(210,4%,10%,.05);
> 
> ah I didn't know that hsl was an alias for hsla (or vice versa). Might be
> more consistent to use hsla() here for consistency.

Good idea.

(In reply to Brian Birtles (:birtles, traveling 29 June - 6 July) from comment #109)
> (In reply to Julian Descottes [:jdescottes][:julian] from comment #98)
> > ::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:458
> > (Diff revision 2)
> > > +      anchorWin
> > > +        .getComputedStyle(anchor)
> > 
> > Maybe we can use a single anchorWin.getComputedStyle(anchor) for isRtl and
> > for borderRadius?
> 
> From a performance point of view, it doesn't make any difference.
> getComputedStyle() itself doesn't do the work of flushing style/layout --
> accessing its properties does.

Never mind this, I can see that re-using the gCS object makes the code shorter.

Comment 111

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #99)
> Comment on attachment 8988397 [details]
> Bug 1461522 - Factor out a focusableSelector utility;
> 
> https://reviewboard.mozilla.org/r/253684/#review261008
> 
> Works for me. 
> I could see this file move to devtools/shared/css/focus.js (or
> focusable-selector.js). 
> But both folders are a bit messy at the moment so no strong preference.

Oh I didn't know about the css folder. Thinking about it though, I'd rather keep this where it is since I see this file as being more related to HTML/DOM and the selector simply being a particular API to that logic. That is, I could well imagine us adding a function to this file that returns the corresponding NodeList instead of the selector, at which point putting this file under 'css' wouldn't make sense.

Comment 112

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #101)
> Comment on attachment 8988398 [details]
> Bug 1461522 - Add focus() and focusEnd() methods to HTMLTooltip;
> 
> https://reviewboard.mozilla.org/r/253686/#review261012
> 
> Thanks!
> 
> ::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:549
> (Diff revision 2)
> >      // Keep a pointer on the focused element to refocus it when hiding the tooltip.
> >      this._focusedElement = this.doc.activeElement;
> >  
> >      this.doc.defaultView.clearTimeout(this.attachEventsTimer);
> >      this.attachEventsTimer = this.doc.defaultView.setTimeout(() => {
> > -      this._maybeFocusTooltip();
> > +      if (this.autofocus) {
> 
> This "autofocus" feature was initially meant for the Debugger variable view
> tooltip which was never migrated to HTML. I wonder if we should remove it.

Filed bug 1472931 for that.

Comment 113

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988399 [details]
Bug 1461522 - Add ID and class properties to HTMLTooltip;

https://reviewboard.mozilla.org/r/253688/#review261220

::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:289
(Diff revision 2)
> + *        - {String} className
> + *          Additional classes to add to the tooltip container element.

Fixed the comment.

The className is not currently used. It was in a previous version of the patch and then I figured it was useful to have so I left it. Let me know if we should drop it.

Comment 114

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #106)
> Comment on attachment 8988402 [details]
> Bug 1461522 - Add Menu components;
> 
> https://reviewboard.mozilla.org/r/253694/#review261048
> 
> Thanks for the patch! There is clear overlap between this new menu and the
> one provided by client/framework./menu.js
> We should clarify this either with better comments or different names to
> explain when and why you should pick one over the other.
> Could you file a follow up for this?

Filed bug 1472936 for this.

Comment 115

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988402 [details]
Bug 1461522 - Add Menu components;

https://reviewboard.mozilla.org/r/253694/#review261048

> it's a bit sad that this is our only way to identify a keyboard-triggered click event.

Right, or at least the alternative is basically to do all our own keyboard handling rather than re-using the native handling we get with <button> elements.

> We rarely use assert() in client code, so just making sure you left this intentionally?

Yeah, I wasn't aware about the aversion to assert. I'm happy to drop it.

> Here we are completely bypassing the call to setContent for this tooltip. We should make this a bit more explicit with a comment. 
> 
> And maybe file a follow up to make this the only supported behavior of the tooltip: every caller would set the content via tooltip.panel.appendChild and would set { width, height } either via another method or via additional parameters in show()?

Yeah, I wasn't really sure what the purpose of setContent was. Dropping it sounds good to me. Filed bug 1472942 for this.

> Why use a function for `ref` here rather than setting ref: "buttonRef" and accessing it via this.refs.buttonRef ?
> 
> Same question applies to the other 2 components

That's the old old React API which will be deprecated.[1] (If only we had already updated to React 16.3 we could go straight ahead and use createRef.)

[1] https://reactjs.org/docs/refs-and-the-dom.html#legacy-api-string-refs

> we could bail out early if !focusIsInList() rather than having it in each `case`?

Yeah, we could definitely do that. It used to do that, but at one point there was some cases where we needed to handle the event anyway. I guess that went away though.

Comment 116

[Julian Descottes [:jdescottes]]

(In reply to Brian Birtles (:birtles, traveling 29 June - 6 July) from comment #109)
> (In reply to Julian Descottes [:jdescottes][:julian] from comment #98)
> > ::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:189
> > (Diff revision 2)
> > > +  //
> > > +  // So for those we need to check if the anchor is more right or left.
> > > +  let hangDirection;
> > > +  if (type === TYPE.DOORHANGER) {
> > > +    const anchorCenter = anchorRect.left + anchorRect.width / 2;
> > > +    const viewCenter = windowRect.left + windowRect.width / 2;
> > 
> > I am not sure about using this new "windowRect" to compute the
> > hangDirection. 
> > 
> > Here we make the assumption that the toolbox window is always the reference
> > element (ie "the view"  from the UX guidelines). Would that still be true if
> > we had a doorhanger inside an inspector sidepanel for instance (eg animation
> > inspector)? 
> > 
> > An alternative would be to let the caller pass the desired hangDirection for
> > doorhanger tooltips.
> 
> The doorhanger position needs to depend on where the anchor is in relation
> to the window. So, for example, if we use this for the chevron button the
> hang direction will depend on how far along the chevron is positioned, which
> side it is docked etc. If the toolbox is docked to the left, it should hang
> to the right etc. So I don't think we can pass this in without putting a lot
> of burden on each call site. My read of the UX guidelines based on testing
> how other doorhangers work is that this should be relative to the outermost
> window (although perhaps I've chosen the wrong window? It seemed right when
> I tested).
> 

Yes you are right, I don't why, when reading this patch I thought window was only the toolbox window. 

> > ::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:460
> > (Diff revision 2)
> > >      const isRtl = anchorWin.getComputedStyle(anchor).direction === "rtl";
> > > +
> > > +    let borderRadius = parseFloat(
> > > +      anchorWin
> > > +        .getComputedStyle(anchor)
> > > +        .getPropertyValue("--theme-arrowpanel-border-radius")
> > 
> > I think borderRadius should always be 0 for ARROW and NORMAL types. ARROW
> > type has a platform agnostic 3px border radius, but it's already "handled"
> > via the ARROW_OFFSET.
> 
> Yes. Long-term I'd like to get rid of ARROW_OFFSET etc. and get everything
> from CSS variables in computed style so I was trying to move that direction
> in this patch. Perhaps that can come later though if you're concerned about
> perf here.

I am not concerned about perf. With this the arrow position for the ARROW type tooltip will be different on some platforms compared to what it used to be. Also it feels a bit strange to read a doorhanger-specific CSS variable and pass it as the "borderRadius" regardless of the tooltip type. 


(In reply to Brian Birtles (:birtles, traveling 29 June - 6 July) from comment #110)
> (In reply to Julian Descottes [:jdescottes][:julian] from comment #98)
> > Comment on attachment 8988396 [details]
> > Bug 1461522 - Add doorhanger type to HTMLTooltip;
> > ...
> > ::: devtools/client/themes/tooltips.css:268
> > (Diff revision 2)
> > >  }
> > >  
> > > +/* Tooltip : doorhanger style */
> > > +
> > > +:root {
> > > +  --theme-arrowpanel-border-radius: 0px;
> > 
> > nit: 0px -> 0
> 
> Why is this?
> 
> (From a CSS point of view, the unitless 0 is generally considered a mistake
> that we would like to remove if possible.)

I thought on the contrary we usually tried to avoid units for 0? I remember having a CSS linter enforcing that, so I would be interested in seeing references related to this removal. 

Now for this particular case, that's what we do in most of our CSS as well as in the rest of your patch, but if you prefer to keep it I don't feel strongly about it. 

> 
> > ::: devtools/client/themes/tooltips.css:268
> > (Diff revision 2)
> > >  }
> > >  
> > > +/* Tooltip : doorhanger style */
> > > +
> > > +:root {
> > > +  --theme-arrowpanel-border-radius: 0px;
> > 
> > As a general note: why use the term arrowpanel rather than doorhanger here?
> 
> I was trying to match the terminology elsewhere in Firefox. Is that ok?
> 
> > ::: devtools/client/themes/tooltips.css:392
> > (Diff revision 2)
> > > +  /* Override the display: -moz-box declaration in minimal-xul.css
> > > +   * or else menu items won't stack. */
> > > +  display: block;
> > > +}
> > > +
> > > +.tooltip-container[type="doorhanger"] li > .command {
> > 
> > Up to you, but maybe it could be nice to come up with a dedicated classname
> > for the `li` of the doorhanger menu? 
> 
> Yeah, .menuitem or something like that?
> 
> > And I wonder if all the styles really specific to the content could be moved
> > to a dedicated CSS file? That's what we do for the various editors in the
> > inspector, they all have their dedicated CSS file (eg
> > devtools/client/shared/widgets/filter-widget.css) which are imported at the
> > beginning of tooltips.css.
> 
> Yeah, all these are supposed to be generic menu styles (not specific to the
> meatball menu). Originally I named the "doorhanger" type the "menu" type and
> it made sense at that point to include them here but now that we're calling
> it "doorhanger" it probably makes sense to move these to a separate file.
> 
> > Of course this is a good candidate for a follow-up
> 
> Filed bug 1472904 for that.
> 
> > ::: devtools/client/themes/variables.css:35
> > (Diff revision 2)
> > >    --theme-toolbar-background-hover: rgba(221, 225, 228, 0.66);
> > >    --theme-toolbar-background-alt: #f5f5f5;
> > >    --theme-toolbar-hover: var(--grey-20);
> > >    --theme-toolbar-hover-active: var(--grey-20);
> > >  
> > > +  /* Doorhangers */
> > 
> > I am always unsure about where to put CSS variables in DevTools, those might
> > be more appropriate in tooltips.css? 
> 
> Yeah, I wasn't sure. I got the impression that we tried to put most of the
> theme-specific variables in variables.css so it was easier to tweaks themes
> in one place.
> 
> > I would in any case have them after the more generic variables, eg after
> > "--theme-tooltip-shadow".
> 
> Sure.
> 
> > ::: devtools/client/themes/variables.css:37
> > (Diff revision 2)
> > >    --theme-toolbar-hover: var(--grey-20);
> > >    --theme-toolbar-hover-active: var(--grey-20);
> > >  
> > > +  /* Doorhangers */
> > > +  --theme-arrowpanel-background: white;
> > > +  --theme-arrowpanel-color: -moz-fieldText;
> > 
> > Can we use an existing variable instead?
> 
> I'm not sure I follow. What's the advantage of that? Doesn't it lead to
> changes having unforeseen consequences?

Maybe I'm overlooking something, but we try to reuse photon or theme variables as much as possible, that's why I was prompted to comment here.

We have several options: 
- use a photon color variable
- use a generic DevTools theme variable

I was more thinking about the second option, and if we still consider DevTools themeing as something we want to pursue, I think this makes it easier? -moz-fieldText is not in any way related to a DevTools theme. Now maybe we don't want the doorhanger menus to actually follow themes? In which case we could use a comment here.

> 
> > ::: devtools/client/themes/variables.css:42
> > (Diff revision 2)
> > > +  --theme-arrowpanel-color: -moz-fieldText;
> > > +  --theme-arrowpanel-border-color: var(--grey-90-a20);
> > > +  --theme-arrowpanel-separator: var(--grey-90-a20);
> > > +  --theme-arrowpanel-dimmed: hsla(0,0%,80%,.3);
> > > +  --theme-arrowpanel-dimmed-further: hsla(0,0%,80%,.45);
> > > +  --theme-arrowpanel-disabled-color: GrayText;
> > 
> > Any reason to use GrayText over an existing "disabled" DevTools variable eg
> > --theme-body-color-inactive?
> 
> As before (and also to mirror the usage elsewhere in Firefox). Let me know
> what you prefer here.
> 

Same comment.

> > ::: devtools/client/themes/variables.css:113
> > (Diff revision 2)
> > >  
> > >    --theme-codemirror-gutter-background: #f4f4f4;
> > >    --theme-messageCloseButtonFilter: invert(0);
> > >  }
> > >  
> > > +:root[platform="mac"].theme-light {
> > 
> > Is there any relevant comment we could add to understand why we need
> > different colors for OSX?
> 
> Yeah, although it's basically just "Because that's what we do elsewhere in
> Firefox".
> 

A searchfox permalink to another spot that does this would be enough in this case.
 
> > ::: devtools/client/themes/variables.css:115
> > (Diff revision 2)
> > >    --theme-messageCloseButtonFilter: invert(0);
> > >  }
> > >  
> > > +:root[platform="mac"].theme-light {
> > > +  --theme-arrowpanel-color: rgb(26,26,26);
> > > +  --theme-arrowpanel-border-color: hsl(210,4%,10%,.05);
> > 
> > ah I didn't know that hsl was an alias for hsla (or vice versa). Might be
> > more consistent to use hsla() here for consistency.
> 
> Good idea.
> 
> (In reply to Brian Birtles (:birtles, traveling 29 June - 6 July) from
> comment #109)
> > (In reply to Julian Descottes [:jdescottes][:julian] from comment #98)
> > > ::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:458
> > > (Diff revision 2)
> > > > +      anchorWin
> > > > +        .getComputedStyle(anchor)
> > > 
> > > Maybe we can use a single anchorWin.getComputedStyle(anchor) for isRtl and
> > > for borderRadius?
> > 
> > From a performance point of view, it doesn't make any difference.
> > getComputedStyle() itself doesn't do the work of flushing style/layout --
> > accessing its properties does.
> 
> Never mind this, I can see that re-using the gCS object makes the code
> shorter.

Yes that was the only reason I mentioned it :)

Comment 117

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988402 [details]
Bug 1461522 - Add Menu components;

https://reviewboard.mozilla.org/r/253694/#review261048

> If the tooltip is visible, destroy() will call hide() which will emit a "hidden" event asynchronously. So we might remove the onHidden listener before the event is fired. Should we update the state to set expanded to false manually and synchronously here?

Unfortunately setState doesn't offer a synchronous API. We can use the callback form of setState and make resetTooltip async but given that ways that resetTooltip is used, I don't think we actually need to block on the state updating.

Comment 118

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988392 [details]
Bug 1461522 - Make HTMLTooltip position the arrow correctly in RTL mode;

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

Comment 119

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988393 [details]
Bug 1461522 - Use the currentZoom as opposed to the pref value;

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

Comment 120

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988394 [details]
Bug 1461522 - Fix some comments in HTMLTooltip.js;

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

Comment 121

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988395 [details]
Bug 1461522 - Allow { height: "auto" } in HTMLTooltip setContent and make it the default;

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

Comment 122

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988396 [details]
Bug 1461522 - Add doorhanger type to HTMLTooltip;

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

Comment 123

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988397 [details]
Bug 1461522 - Factor out a focusableSelector utility;

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

Comment 124

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988398 [details]
Bug 1461522 - Add focus() and focusEnd() methods to HTMLTooltip;

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

Comment 125

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988399 [details]
Bug 1461522 - Add ID and class properties to HTMLTooltip;

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

Comment 126

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988400 [details]
Bug 1461522 - Honor ui.popup.disable_autohide setting in HTMLTooltip;

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

Comment 127

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988401 [details]
Bug 1461522 - Add a mechanism to allow updating an HTMLTooltip's size and position;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253692/diff/4-5/

Comment 128

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988402 [details]
Bug 1461522 - Add Menu components;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253694/diff/4-5/

Comment 129

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988403 [details]
Bug 1461522 - Persist active styles for a button with an expanded menu;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253696/diff/4-5/

Comment 130

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988404 [details]
Bug 1461522 - Update browser_toolbox_zoom_popup.js to handle doorhanger menus too;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253698/diff/4-5/

Comment 131

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988405 [details]
Bug 1461522 - Update comments in ToolboxToolbar.js;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253700/diff/4-5/

Comment 132

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988406 [details]
Bug 1461522 - Switch meatball menu to a doorhanger;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253702/diff/4-5/

Comment 133

[Brian Birtles (:birtles, Feb 26~ parental leave)]

> > > ::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:460
> > > (Diff revision 2)
> > > >      const isRtl = anchorWin.getComputedStyle(anchor).direction === "rtl";
> > > > +
> > > > +    let borderRadius = parseFloat(
> > > > +      anchorWin
> > > > +        .getComputedStyle(anchor)
> > > > +        .getPropertyValue("--theme-arrowpanel-border-radius")
> > > 
> > > I think borderRadius should always be 0 for ARROW and NORMAL types. ARROW
> > > type has a platform agnostic 3px border radius, but it's already "handled"
> > > via the ARROW_OFFSET.
> > 
> > Yes. Long-term I'd like to get rid of ARROW_OFFSET etc. and get everything
> > from CSS variables in computed style so I was trying to move that direction
> > in this patch. Perhaps that can come later though if you're concerned about
> > perf here.
> 
> I am not concerned about perf. With this the arrow position for the ARROW
> type tooltip will be different on some platforms compared to what it used to
> be. Also it feels a bit strange to read a doorhanger-specific CSS variable
> and pass it as the "borderRadius" regardless of the tooltip type. 

Oh, I see now. I'll fix that.

> > > ::: devtools/client/themes/variables.css:37
> > > (Diff revision 2)
> > > >    --theme-toolbar-hover: var(--grey-20);
> > > >    --theme-toolbar-hover-active: var(--grey-20);
> > > >  
> > > > +  /* Doorhangers */
> > > > +  --theme-arrowpanel-background: white;
> > > > +  --theme-arrowpanel-color: -moz-fieldText;
> > > 
> > > Can we use an existing variable instead?
> > 
> > I'm not sure I follow. What's the advantage of that? Doesn't it lead to
> > changes having unforeseen consequences?
> 
> Maybe I'm overlooking something, but we try to reuse photon or theme
> variables as much as possible, that's why I was prompted to comment here.
> 
> We have several options: 
> - use a photon color variable
> - use a generic DevTools theme variable
> 
> I was more thinking about the second option, and if we still consider
> DevTools themeing as something we want to pursue, I think this makes it
> easier? -moz-fieldText is not in any way related to a DevTools theme. Now
> maybe we don't want the doorhanger menus to actually follow themes? In which
> case we could use a comment here.

I understand about the photon color variables and I'll see what I can find there.

For the rest I tried to match doorhangers used elsewhere in Firefox as best as possible (including Firefox's dark theme). -moz-fieldText is difficult. That's what we use for doorhangers elsewhere in Firefox (but not on Mac) and I think the DevTools should follow that. I'm not sure what else to do there? We could introduce another level of indirection for this, but I wonder where else it would be used?

I guess I'm still unsure how this should look.

> > > ::: devtools/client/themes/variables.css:113
> > > (Diff revision 2)
> > > >  
> > > >    --theme-codemirror-gutter-background: #f4f4f4;
> > > >    --theme-messageCloseButtonFilter: invert(0);
> > > >  }
> > > >  
> > > > +:root[platform="mac"].theme-light {
> > > 
> > > Is there any relevant comment we could add to understand why we need
> > > different colors for OSX?
> > 
> > Yeah, although it's basically just "Because that's what we do elsewhere in
> > Firefox".
> > 
> 
> A searchfox permalink to another spot that does this would be enough in this
> case.

I added a comment just guessing why we do things differently for Mac. It might take until next week (when I have access to a Mac) to work out exactly what rule is used there and find a better reason.

Comment 134

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988392 [details]
Bug 1461522 - Make HTMLTooltip position the arrow correctly in RTL mode;

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

Comment 135

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988393 [details]
Bug 1461522 - Use the currentZoom as opposed to the pref value;

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

Comment 136

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988394 [details]
Bug 1461522 - Fix some comments in HTMLTooltip.js;

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

Comment 137

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988395 [details]
Bug 1461522 - Allow { height: "auto" } in HTMLTooltip setContent and make it the default;

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

Comment 138

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988396 [details]
Bug 1461522 - Add doorhanger type to HTMLTooltip;

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

Comment 139

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988397 [details]
Bug 1461522 - Factor out a focusableSelector utility;

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

Comment 140

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988398 [details]
Bug 1461522 - Add focus() and focusEnd() methods to HTMLTooltip;

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

Comment 141

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988399 [details]
Bug 1461522 - Add ID and class properties to HTMLTooltip;

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

Comment 142

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988400 [details]
Bug 1461522 - Honor ui.popup.disable_autohide setting in HTMLTooltip;

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

Comment 143

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988401 [details]
Bug 1461522 - Add a mechanism to allow updating an HTMLTooltip's size and position;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253692/diff/5-6/

Comment 144

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988402 [details]
Bug 1461522 - Add Menu components;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253694/diff/5-6/

Comment 145

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988403 [details]
Bug 1461522 - Persist active styles for a button with an expanded menu;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253696/diff/5-6/

Comment 146

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988404 [details]
Bug 1461522 - Update browser_toolbox_zoom_popup.js to handle doorhanger menus too;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253698/diff/5-6/

Comment 147

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988405 [details]
Bug 1461522 - Update comments in ToolboxToolbar.js;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253700/diff/5-6/

Comment 148

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988406 [details]
Bug 1461522 - Switch meatball menu to a doorhanger;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253702/diff/5-6/

Comment 149

[Julian Descottes [:jdescottes]]

Comment on attachment 8988394 [details]
Bug 1461522 - Fix some comments in HTMLTooltip.js;

https://reviewboard.mozilla.org/r/253678/#review261394

::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:349
(Diff revision 4)
> -   *          If layout permits, the tooltip will be displayed on top/bottom
> -   *          of the anchor. If ommitted, the tooltip will be displayed where
> -   *          more space is available.
> -   *        - {Number} x: optional, horizontal offset between the anchor and the tooltip
> -   *        - {Number} y: optional, vertical offset between the anchor and the tooltip
> +   * @param {String} options.position
> +   *        Optional, possible values: top|bottom
> +   *        If layout permits, the tooltip will be displayed on top/bottom
> +   *        of the anchor. If omitted, the tooltip will be displayed where
> +   *        more space is available.
> +   * @param {Number} options.x
> +   *        Optional, horizontal offset between the anchor and the tooltip.
> +   * @param {Number} options.y
> +   *        Optional, vertical offset between the anchor and the tooltip.

No particular preference for either style but I have never seen this for destructured parameters in DevTools codebase. Is that more frequent in other parts of the Firefox codebase?

Comment 150

[Julian Descottes [:jdescottes]]

Comment on attachment 8988404 [details]
Bug 1461522 - Update browser_toolbox_zoom_popup.js to handle doorhanger menus too;

https://reviewboard.mozilla.org/r/253698/#review261404

::: devtools/client/framework/test/browser_toolbox_zoom_popup.js:134
(Diff revision 6)
> +    menuPopup = doc.getElementById(menuButton.getAttribute("aria-controls"));
> +    await waitUntil(() => menuPopup.classList.contains("tooltip-visible"));
> +  } else {
> +    menuType = "native";
> +    const popupset = doc.querySelector("popupset");
> +    menuPopup = popupset.querySelector("panel[panelopen=\"true\"]");

This assignment will always be overridden by the one in the wait until, so it should be removed. Did you intend to remove the assignment from the waitUntil instead?

Comment 151

[Julian Descottes [:jdescottes]]

Comment on attachment 8988406 [details]
Bug 1461522 - Switch meatball menu to a doorhanger;

https://reviewboard.mozilla.org/r/253702/#review261416

In the last version, the UP/DOWN keyboard navigation no longer works for me, but I'm pretty sure it was working with a previous revision.
It would be nice to fix this before landing.

One comment about one of your prior questions:

> * I matched the behavior of other menus (hamburger menu, bookmark menu)
> where clicking the menu
>   button while the menu is open re-opens it, but I feel like that's not
> intuitive for the meatball

I don't have this behavior for hamburger and bookmark menus. When clicking on the menu-button while open, they close. 
We can fix this in a follow up.

We should also make sure we have a follow up to add tests as well. We don't have yet a good solution for unit testing react component.
So for now we'll have to do this with a mochitest, probably relying on keyboard navigation as you mentioned.

And once again, congratulations on getting through this! The new menu looks much nicer, and you managed to integrate HTMLTooltip and React in a nice way.

::: devtools/client/framework/components/MeatballMenu.js:58
(Diff revision 6)
> +
> +      // Function to turn the disable pop-up autohide behavior on / off.
> +      toggleNoAutohide: PropTypes.func,
> +
> +      // Localization interface.
> +      L10N: PropTypes.object,

this is required.

::: devtools/client/framework/components/MeatballMenu.js:61
(Diff revision 6)
> +
> +      // Localization interface.
> +      L10N: PropTypes.object,
> +
> +      // The devtools toolbox
> +      toolbox: PropTypes.object,

I don't think this is used

::: devtools/client/framework/components/MeatballMenu.js:74
(Diff revision 6)
> +  componentDidUpdate(prevProps) {
> +    if (!this.props.onResize) {
> +      return;
> +    }
> +
> +    // We are only expecting two kinds of dynamic changes when a popup is

nit: two -> three (or something future proof eg "the following")

::: devtools/client/framework/components/MeatballMenu.js:101
(Diff revision 6)
> +    const items = [];
> +
> +    // Dock options
> +    for (const hostType of this.props.hostTypes) {
> +      const l10nkey =
> +        hostType.position === "window" ? "separateWindow" : hostType.position;

This code was just moved so I don't want to block landing on this, but in general I prefer to avoid string concatenation for l10n keys. More verbose, but we really have no tooling to find where a given l10n string is used, unless the code uses the exact same string.

Also this kind of code tends to become messy really quickly. As soon as a string is updated and we add the typical "2" suffix, this kind of code usually morphs into `if (type === "something") { key = key + "2"}`. Example: https://searchfox.org/mozilla-central/source/devtools/client/storage/ui.js#992-995

::: devtools/client/framework/components/MeatballMenu.js:166
(Diff revision 6)
> +        onClick: () => this.props.toggleOptions(),
> +        className: "iconic",
> +      })
> +    );
> +
> +    if (items.length) {

This can never be false so maybe we can remove the if?

::: devtools/client/framework/components/ToolboxToolbar.js:294
(Diff revision 6)
> +      doc: toolbox.doc,
> -    onFocus: () => focusButton(meatballMenuButtonId),
> +      onFocus: () => focusButton(meatballMenuButtonId),
> -    className: "devtools-button",
> +      className: "devtools-button",
> -    title: L10N.getStr("toolbox.meatballMenu.button.tooltip"),
> +      title: L10N.getStr("toolbox.meatballMenu.button.tooltip"),
> -    onClick: evt => {
> -      showMeatballMenu(evt.target, {
> +      tabIndex: focusedButton === meatballMenuButtonId ? "0" : "-1",
> +      ref: "meatballMenu",

Maybe this should be named meatballMenuButton to avoid the confusion with its meatballMenu child?

Comment 152

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #149)
> Comment on attachment 8988394 [details]
> Bug 1461522 - Fix some comments in HTMLTooltip.js;
> 
> https://reviewboard.mozilla.org/r/253678/#review261394
> 
> ::: devtools/client/shared/widgets/tooltip/HTMLTooltip.js:349
> (Diff revision 4)
> > -   *          If layout permits, the tooltip will be displayed on top/bottom
> > -   *          of the anchor. If ommitted, the tooltip will be displayed where
> > -   *          more space is available.
> > -   *        - {Number} x: optional, horizontal offset between the anchor and the tooltip
> > -   *        - {Number} y: optional, vertical offset between the anchor and the tooltip
> > +   * @param {String} options.position
> > +   *        Optional, possible values: top|bottom
> > +   *        If layout permits, the tooltip will be displayed on top/bottom
> > +   *        of the anchor. If omitted, the tooltip will be displayed where
> > +   *        more space is available.
> > +   * @param {Number} options.x
> > +   *        Optional, horizontal offset between the anchor and the tooltip.
> > +   * @param {Number} options.y
> > +   *        Optional, vertical offset between the anchor and the tooltip.
> 
> No particular preference for either style but I have never seen this for
> destructured parameters in DevTools codebase. Is that more frequent in other
> parts of the Firefox codebase?

Yeah, I started using this in a previous patch since I wasn't sure how best to do it and I just followed the standard jsdoc format[1] in case we want to run any tooling across these docs at any point.

[1] http://usejsdoc.org/tags-param.html#parameters-with-properties

Comment 153

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #150)
> Comment on attachment 8988404 [details]
> Bug 1461522 - Update browser_toolbox_zoom_popup.js to handle doorhanger
> menus too;
> 
> https://reviewboard.mozilla.org/r/253698/#review261404
> 
> ::: devtools/client/framework/test/browser_toolbox_zoom_popup.js:134
> (Diff revision 6)
> > +    menuPopup = doc.getElementById(menuButton.getAttribute("aria-controls"));
> > +    await waitUntil(() => menuPopup.classList.contains("tooltip-visible"));
> > +  } else {
> > +    menuType = "native";
> > +    const popupset = doc.querySelector("popupset");
> > +    menuPopup = popupset.querySelector("panel[panelopen=\"true\"]");
> 
> This assignment will always be overridden by the one in the wait until, so
> it should be removed. Did you intend to remove the assignment from the
> waitUntil instead?

Oh, good catch. I have no idea where that line came from. I'll drop it.

Comment 154

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #151)
> Comment on attachment 8988406 [details]
> Bug 1461522 - Switch meatball menu to a doorhanger;
> 
> https://reviewboard.mozilla.org/r/253702/#review261416
> 
> In the last version, the UP/DOWN keyboard navigation no longer works for me,
> but I'm pretty sure it was working with a previous revision.
> It would be nice to fix this before landing.

Yes, looks like I broke it yesterday. It should be fixed in the next version I push.

> One comment about one of your prior questions:
> 
> > * I matched the behavior of other menus (hamburger menu, bookmark menu)
> > where clicking the menu
> >   button while the menu is open re-opens it, but I feel like that's not
> > intuitive for the meatball
> 
> I don't have this behavior for hamburger and bookmark menus. When clicking
> on the menu-button while open, they close. 
> We can fix this in a follow up.

Yes, I see that on Windows too it doesn't re-open it. It does on Linux however.

Filed bug 1473209 to fix this.
 
> We should also make sure we have a follow up to add tests as well. We don't
> have yet a good solution for unit testing react component.
> So for now we'll have to do this with a mochitest, probably relying on
> keyboard navigation as you mentioned.

Filed bug 1473210 for this.

> And once again, congratulations on getting through this! The new menu looks
> much nicer, and you managed to integrate HTMLTooltip and React in a nice way.

Thanks!

> ::: devtools/client/framework/components/MeatballMenu.js:101
> (Diff revision 6)
> > +    const items = [];
> > +
> > +    // Dock options
> > +    for (const hostType of this.props.hostTypes) {
> > +      const l10nkey =
> > +        hostType.position === "window" ? "separateWindow" : hostType.position;
> 
> This code was just moved so I don't want to block landing on this, but in
> general I prefer to avoid string concatenation for l10n keys. More verbose,
> but we really have no tooling to find where a given l10n string is used,
> unless the code uses the exact same string.
> 
> Also this kind of code tends to become messy really quickly. As soon as a
> string is updated and we add the typical "2" suffix, this kind of code
> usually morphs into `if (type === "something") { key = key + "2"}`. Example:
> https://searchfox.org/mozilla-central/source/devtools/client/storage/ui.
> js#992-995

That makes sense. I've filed bug 1473211 for this since I'd prefer for this patch to be primarily about moving code.

Comment 155

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988392 [details]
Bug 1461522 - Make HTMLTooltip position the arrow correctly in RTL mode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253674/diff/4-5/

Comment 156

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988393 [details]
Bug 1461522 - Use the currentZoom as opposed to the pref value;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253676/diff/4-5/

Comment 157

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988394 [details]
Bug 1461522 - Fix some comments in HTMLTooltip.js;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253678/diff/4-5/

Comment 158

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988395 [details]
Bug 1461522 - Allow { height: "auto" } in HTMLTooltip setContent and make it the default;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253680/diff/4-5/

Comment 159

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988396 [details]
Bug 1461522 - Add doorhanger type to HTMLTooltip;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253682/diff/4-5/

Comment 160

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988397 [details]
Bug 1461522 - Factor out a focusableSelector utility;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253684/diff/4-5/

Comment 161

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988398 [details]
Bug 1461522 - Add focus() and focusEnd() methods to HTMLTooltip;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253686/diff/4-5/

Comment 162

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988399 [details]
Bug 1461522 - Add ID and class properties to HTMLTooltip;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253688/diff/4-5/

Comment 163

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988400 [details]
Bug 1461522 - Honor ui.popup.disable_autohide setting in HTMLTooltip;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253690/diff/4-5/

Comment 164

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988401 [details]
Bug 1461522 - Add a mechanism to allow updating an HTMLTooltip's size and position;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253692/diff/6-7/

Comment 165

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988402 [details]
Bug 1461522 - Add Menu components;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253694/diff/6-7/

Comment 166

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988403 [details]
Bug 1461522 - Persist active styles for a button with an expanded menu;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253696/diff/6-7/

Comment 167

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988404 [details]
Bug 1461522 - Update browser_toolbox_zoom_popup.js to handle doorhanger menus too;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253698/diff/6-7/

Comment 168

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988405 [details]
Bug 1461522 - Update comments in ToolboxToolbar.js;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253700/diff/6-7/

Comment 169

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8988406 [details]
Bug 1461522 - Switch meatball menu to a doorhanger;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253702/diff/6-7/

Comment 170

[Brian Birtles (:birtles, Feb 26~ parental leave)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53ec00d2889068d1ea80bd8bd114c326a571f3f7

Comment 171

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f995e3ff5de4
Make HTMLTooltip position the arrow correctly in RTL mode; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/52628cf763c9
Use the currentZoom as opposed to the pref value; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/924dcb50f6da
Fix some comments in HTMLTooltip.js; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/95fe4a4625fe
Allow { height: "auto" } in HTMLTooltip setContent and make it the default; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/42d6ff342171
Add doorhanger type to HTMLTooltip; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/dd9b63632e19
Factor out a focusableSelector utility; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/c4f83d8e4596
Add focus() and focusEnd() methods to HTMLTooltip; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/28e68f45879e
Add ID and class properties to HTMLTooltip; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/7ff43e3acaf7
Honor ui.popup.disable_autohide setting in HTMLTooltip; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/0dead7ce127b
Add a mechanism to allow updating an HTMLTooltip's size and position; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/31e35c02f84b
Add Menu components; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/6670a1258320
Persist active styles for a button with an expanded menu; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/1c4c3998796c
Update browser_toolbox_zoom_popup.js to handle doorhanger menus too; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/d6badfe12433
Update comments in ToolboxToolbar.js; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/cc6ec2789c9d
Switch meatball menu to a doorhanger; r=jdescottes

Comment 172

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/f995e3ff5de4
https://hg.mozilla.org/mozilla-central/rev/52628cf763c9
https://hg.mozilla.org/mozilla-central/rev/924dcb50f6da
https://hg.mozilla.org/mozilla-central/rev/95fe4a4625fe
https://hg.mozilla.org/mozilla-central/rev/42d6ff342171
https://hg.mozilla.org/mozilla-central/rev/dd9b63632e19
https://hg.mozilla.org/mozilla-central/rev/c4f83d8e4596
https://hg.mozilla.org/mozilla-central/rev/28e68f45879e
https://hg.mozilla.org/mozilla-central/rev/7ff43e3acaf7
https://hg.mozilla.org/mozilla-central/rev/0dead7ce127b
https://hg.mozilla.org/mozilla-central/rev/31e35c02f84b
https://hg.mozilla.org/mozilla-central/rev/6670a1258320
https://hg.mozilla.org/mozilla-central/rev/1c4c3998796c
https://hg.mozilla.org/mozilla-central/rev/d6badfe12433
https://hg.mozilla.org/mozilla-central/rev/cc6ec2789c9d

Comment 174

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/f995e3ff5de4
https://hg.mozilla.org/mozilla-central/rev/52628cf763c9
https://hg.mozilla.org/mozilla-central/rev/924dcb50f6da
https://hg.mozilla.org/mozilla-central/rev/95fe4a4625fe
https://hg.mozilla.org/mozilla-central/rev/42d6ff342171
https://hg.mozilla.org/mozilla-central/rev/dd9b63632e19
https://hg.mozilla.org/mozilla-central/rev/c4f83d8e4596
https://hg.mozilla.org/mozilla-central/rev/28e68f45879e
https://hg.mozilla.org/mozilla-central/rev/7ff43e3acaf7
https://hg.mozilla.org/mozilla-central/rev/0dead7ce127b
https://hg.mozilla.org/mozilla-central/rev/31e35c02f84b
https://hg.mozilla.org/mozilla-central/rev/6670a1258320
https://hg.mozilla.org/mozilla-central/rev/1c4c3998796c
https://hg.mozilla.org/mozilla-central/rev/d6badfe12433
https://hg.mozilla.org/mozilla-central/rev/cc6ec2789c9d