Tim Nguyen :ntim Reporter
	Description • 10 years ago

Why don't we use the australis widget styling ? FYI our goal is consistency.

	Florens Verschelde :fvsch
	Comment 1 • 10 years ago

Quick mock-up I did taking the bookmarks dropdown as a reference. I don’t know if it’s the exact look to go for (especially the gray “Manage search engines…”).

	Tim Nguyen :ntim Reporter
	Comment 2 • 10 years ago

(In reply to Florent Verschelde from comment #1)
> Created attachment 8426430 [details]
> Mock-up to make the difference clear
> 
> Quick mock-up I did taking the bookmarks dropdown as a reference. I don’t
> know if it’s the exact look to go for (especially the gray “Manage search
> engines…”).

Perfect ! Exactly what I meant. I think the Manage search engines is good as footer, since Show all Bookmarks also allows managing bookmarks.

	Florens Verschelde :fvsch
	Comment 3 • 10 years ago

I took it on me to try and fix this (my first bug, yay!). So far I switched to a XUL <popupmenu> (instead of panel) to get the keyboard navigation and the right styles when hovering or focusing items in the list. I’m also referencing the styles from chrome://browser/skin/customizableui/panelUIOverlay.css directly, I figured duplicating them wouldn’t be the best solution.
I had a hard time getting the <menuitem>s right, as I didn’t know XUL at all, but it should be okay now.

What stumps me is getting the arrow on top (and no padding in the bottom). I guess it’s a matter of using the right -moz-binding but I’m not sure there’s one that can be used as-is.

	Tim Nguyen :ntim Reporter
	Comment 4 • 10 years ago

(In reply to Florent Verschelde from comment #3)
> Created attachment 8428909 [details]
> Work in progress (screenshot)
> 
> I took it on me to try and fix this (my first bug, yay!).
If you need any help, just ping me here, or ask people on IRC :)

> So far I switched to a XUL <popupmenu> (instead of panel) to get the keyboard navigation and
> the right styles when hovering or focusing items in the list. I’m also
> referencing the styles from
> chrome://browser/skin/customizableui/panelUIOverlay.css directly, I figured
> duplicating them wouldn’t be the best solution.
You're adding lots of useless styles by doing this. Maybe we should ask someone for moving some styles into panel.css (at toolkit level).

> I had a hard time getting the <menuitem>s right, as I didn’t know XUL at
> all, but it should be okay now.
I would rather change the hbox's to some toolbarbutton, and keep the panel.

> What stumps me is getting the arrow on top (and no padding in the bottom). I
> guess it’s a matter of using the right -moz-binding but I’m not sure there’s
> one that can be used as-is.
You can use the bookmark panel binding, although, you'll have to adjust id's and stuff like that

	Florens Verschelde :fvsch
	Comment 5 • 10 years ago

> You're adding lots of useless styles by doing this.
> Maybe we should ask someone for moving some styles into panel.css (at toolkit level).
Noted. I’ll focus on this later, right now I’m trying to get the look and feel and the interactivity right.
By the way I found the appropriate binding: chrome://global/content/bindings/popup.xml#arrowpanel

I got the look and feel right, but I can’t get the interactivity right with the different XUL/binding combinations I’m testing.

panel + hbox -> no focus or keybord use
panel + toolbarbutton -> no focus or keyboard use
menupopup + menuitem -> interactivity alright but no arrow
menupopup with "arrowpanel" binding + menuitem -> arrow and keyboard use alright, but clicks outside the menu don’t hide it, it acts as a modal window.

Is there a particular reason to use a <panel>? This feature is basically a <menulist>, and could be coded as:

<menulist>
  <label>[Current engine’s logo]</label>
  <menupopup>
    <menuitem />
    …
    <menuitem />
  </menupopup>
</menulist>

I actually tried that and I can get it to work, except that the "arrowpanel" necessary to get the arrow style will kill some of interactivity.

I could just style the existing panel right, but I’d like to fix the missing keyboard navigation and problematic accessibility while I’m doing that. So a menupopup seems like the right choice for that.

	Tim Nguyen :ntim Reporter
	Comment 6 • 10 years ago

(In reply to Florent Verschelde from comment #5)
> > You're adding lots of useless styles by doing this.
> > Maybe we should ask someone for moving some styles into panel.css (at toolkit level).
> Noted. I’ll focus on this later, right now I’m trying to get the look and
> feel and the interactivity right.
> By the way I found the appropriate binding:
> chrome://global/content/bindings/popup.xml#arrowpanel
> 
> I got the look and feel right, but I can’t get the interactivity right with
> the different XUL/binding combinations I’m testing.
> 
> panel + hbox -> no focus or keybord use
> panel + toolbarbutton -> no focus or keyboard use
> menupopup + menuitem -> interactivity alright but no arrow
> menupopup with "arrowpanel" binding + menuitem -> arrow and keyboard use
> alright, but clicks outside the menu don’t hide it, it acts as a modal
> window.
> 
> Is there a particular reason to use a <panel>? This feature is basically a
> <menulist>, and could be coded as:
> 
> <menulist>
>   <label>[Current engine’s logo]</label>
>   <menupopup>
>     <menuitem />
>     …
>     <menuitem />
>   </menupopup>
> </menulist>
> 
> I actually tried that and I can get it to work, except that the "arrowpanel"
> necessary to get the arrow style will kill some of interactivity.
> 
> I could just style the existing panel right, but I’d like to fix the missing
> keyboard navigation and problematic accessibility while I’m doing that. So a
> menupopup seems like the right choice for that.

We can focus on this in a follow up bug.

Also, can we move some panelUIOverlay.css styles (panel button, header and footer styles) to toolkit level ? so we can re-use it easily later anywhere. It avoids some code duplication, especially for bugs like this one.

	Florens Verschelde :fvsch
	Comment 7 • 10 years ago

Well for now I have code that fixes the panel’s look, plus keyboard navigation and access to the menu when there is no logo (Bug 1014076, which is a partial duplicate of Bug 1006203). It gets the job done but still has many issues, obviously.

I could break it down as:
1) This bug - Fix popup look. (Might need a separate bug for moving some of the CSS code, and mark it as blocking this.)
2) Bug 1006203 - Show a button with text when there is no logo.
3) Bug 1014076 (or a new, more focused bug) - Accessibility fixes (would enventually overwrite the patch for (1)).

	Florens Verschelde :fvsch
	Comment 8 • 10 years ago

For the panelUIOverlay.css styles, I don’t know the codebase enough to know what could be moved to toolkit level. I can copy just the styles I’m using in a section of newtab/newTab.css, or in a new stylesheet such as newtab/fromPanelUIOverlay.css, maybe it would help?

	Tim Nguyen :ntim Reporter
	Comment 9 • 10 years ago

(In reply to Florent Verschelde from comment #8)
> For the panelUIOverlay.css styles, I don’t know the codebase enough to know
> what could be moved to toolkit level. I can copy just the styles I’m using
> in a section of newtab/newTab.css, or in a new stylesheet such as
> newtab/fromPanelUIOverlay.css, maybe it would help?

Yeah, maybe you can just move it to the newTab.css stylesheet, we can then move things at toolkit level later, to make the component reusable later.

	Tim Nguyen :ntim Reporter
	Comment 10 • 10 years ago

(In reply to Florent Verschelde from comment #7)
> Created attachment 8429442 [details]
> current-progress.png
> 
> Well for now I have code that fixes the panel’s look, plus keyboard
> navigation and access to the menu when there is no logo (Bug 1014076, which
> is a partial duplicate of Bug 1006203). It gets the job done but still has
> many issues, obviously.
> 
> I could break it down as:
> 1) This bug - Fix popup look. (Might need a separate bug for moving some of
> the CSS code, and mark it as blocking this.)
> 2) Bug 1006203 - Show a button with text when there is no logo.
> 3) Bug 1014076 (or a new, more focused bug) - Accessibility fixes (would
> enventually overwrite the patch for (1)).
I would personally do 2) in the actual bug, otherwise it's gonna be messy to review.

Btw, have you created a patch yet ? If not, you can check http://codefirefox.com to learn how.

	:Gijs (he/him)
	Comment 11 • 10 years ago

(In reply to Tim Nguyen [:ntim] from comment #6)
> We can focus on this in a follow up bug.
> 
> Also, can we move some panelUIOverlay.css styles (panel button, header and
> footer styles) to toolkit level ? so we can re-use it easily later anywhere.
> It avoids some code duplication, especially for bugs like this one.

I don't think it makes sense to move this to toolkit. For one, it doesn't need to be in toolkit to be reusable - in fact, it's Firefox-specific, so it doesn't belong in toolkit.

Moreover, I expect that the per-platform CSS that we have for the current contents of the various panelUIOverlay.css won't need to apply to the uniformly styled new tab page, and then there's the fact that it's implemented using different elements... I don't see why it needs to move anywhere.

If necessary you can always try to @import the browser/skin/customizableui/panelUIOverlay.css sheet in the new tab page and apply the subviewbutton class and see how you get on. Without a clear idea of how much "duplicate" code is involved here, it's really hard to say.

	Florens Verschelde :fvsch
	Comment 12 • 10 years ago

For the record I’m not duplicating CSS code, just calling it as:
<?xml-stylesheet href="chrome://browser/skin/customizableui/panelUIOverlay.css" type="text/css"?>
I’m going to keep it like this in my first patch, then. It can always be changed in review.

	Florens Verschelde :fvsch
	Comment 13 • 10 years ago

Kitchen sink patch (before I actually break the changes down in their respective bugs). I’m not proposing it for review (though it can be tested), it’s just to give an idea of what I’ve done.

- Changes the panel to a menupopup with menuitems.
- Uses styles from panelUIOverlay.css
- Uses a trimmed down (no JS) copy of the global/content/bindings/popup.xml#arrowpanel binding, because the JS event handlers and how they show/hide panels conflict with how menupopups show/hide.
- Finally, there are a bunch of changes to the search engine picker button (those will be for bug 1006203, but no worries, I’ll post there with a trimmed down patch).

It might take me two or three days to post the individual patches since I’ll be busy.

	Florens Verschelde :fvsch
	Comment 14 • 10 years ago

Oh, and compared to this I do intend to go back to a <panel>, probably with <toolbarbutton>s as :ntim suggested. Using a <menupopup> is better for usability and accessibility but might require the creation of a real XBL binding (rather than the above hack), so it can be handled in (a) separate bug(s).

	Tim Nguyen :ntim Reporter
	Comment 15 • 10 years ago

(In reply to Florent Verschelde from comment #7)
> Created attachment 8429442 [details]
> current-progress.png
> 
> Well for now I have code that fixes the panel’s look, plus keyboard
> navigation and access to the menu when there is no logo (Bug 1014076, which
> is a partial duplicate of Bug 1006203). It gets the job done but still has
> many issues, obviously.
> 
> I could break it down as:
> 1) This bug - Fix popup look. (Might need a separate bug for moving some of
> the CSS code, and mark it as blocking this.)
> 2) Bug 1006203 - Show a button with text when there is no logo.
> 3) Bug 1014076 (or a new, more focused bug) - Accessibility fixes (would
> enventually overwrite the patch for (1)).

Make sure the menu popup looks good on Windows, it currently have a gray styling.

	Dão Gottwald [:dao]
	Comment 16 • 10 years ago

Comment on attachment 8429605 [details] [diff] [review]
b1011610-newtab-search-picker.patch

chrome://browser/skin/customizableui/panelUIOverlay.css is not a general-purpose stylesheet and shouldn't be applied to newTab.xul.