Add ability to highlight search provider icon within expanded search menu. For this iteration we will implement our standard circle highlight.

*yoink*

Work in progress - dealing with popups is a pain :\ Need to adapt hidePanelAnnotations yet.

*sigh* Getting there... I've improved the logging situation to help with figuring out what's happening in this module, because some of this is getting so complex (thanks, popups!).

Comment on attachment 8501699 [details] [diff] [review] WiP v2 Review of attachment 8501699 [details] [diff] [review]: ----------------------------------------------------------------- It would have been nice for the debugging/style to be in their own patch… I guess I'll have to take another look tomorrow to actually give higher level feedback as I got distracted by all the changes and complexity of our magic menus. Is this patch automatically opening the search engine menu for search engine targets? Is that what most of the complexity is about? ::: browser/modules/UITour.jsm @@ +139,5 @@ > }], > ]), > > + menus: new Map([ > + ["appMenu", function(aWindow) { So we don't need "bookmarks" here? @@ +623,5 @@ > > teardownTour: function(aWindow, aWindowClosing = false) { > + if (aWindowClosing) > + logger.debug(`Tearing down tour, window closing`); > + else I guess you should add braces here too @@ +674,1 @@ > return true; Ditto @@ +752,1 @@ > deferred.reject("The specified target name is not in the allowed set"); I suspect you didn't mean to reject twice. @@ +811,5 @@ > + > + let panel = null; > + if (aMenu == "appMenu") > + panel = aWindow.PanelUI.panel; > + else if (aMenu == "searchBar") Ditto on braces along with other lines you touched. @@ +978,5 @@ > > + // TODO Need to use the correct menu here > + this._setMenuStateForAnnotation(aTarget.node.ownerDocument.defaultView, > + "appMenu", "highlight", > + this.targetIsInAppMenu(aTarget), I guess this call to targetIsInAppMenu should change too if we're not talking about the appMenu. @@ +1114,5 @@ > }, > > showMenu: function(aWindow, aMenuName, aOpenCallback = null) { > + function openMenuButton(aMenuBtn) { > + this.el(aWindow, aMenuName); Where is this.el defined? @@ +1143,5 @@ > } else if (aMenuName == "bookmarks") { > + let menuBtn = aWindow.document.getElementById("bookmarks-menu-button"); > + openMenuButton(menuBtn); > + } else if (aMenuName == "searchEngines") { > + let menuBtn = this.targets.get("searchProvider").query(aWindow.document); Shouldn'tt getTarget do the right thing and give you a |node| property on the result? Or are you doing this so it's not async?

(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #4) > Is this patch automatically opening the search engine menu for search engine > targets? Is that what most of the complexity is about? Yep. Basically the entire point of the bug :) We can already highlight just the search bar. See attachment 8490345 [details]. > Shouldn'tt getTarget do the right thing and give you a |node| property on > the result? Or are you doing this so it's not async? Yea, avoiding async.

(In reply to Blair McBride [:Unfocused] from comment #5) > (In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from > comment #4) > > Is this patch automatically opening the search engine menu for search engine > > targets? Is that what most of the complexity is about? > > Yep. Basically the entire point of the bug :) We can already highlight just > the search bar. See attachment 8490345 [details]. As I replied on IRC, "automatically opening the search engine menu" isn't strictly require for this bug. The ability to open it and highlight within it is. If it's easier to require a showMenu("search…") followed by a showHighlight("someEngine") and ignoring the automatic showing/hiding logic of the menu then I would be fine with that.

Comment on attachment 8501699 [details] [diff] [review] WiP v2 Note that the debug logging changes are going to be painful for Tomasz in bug 1073238 as he has a big patch in progress for that.

(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #7) > Note that the debug logging changes are going to be painful for Tomasz in > bug 1073238 as he has a big patch in progress for that. OK, I'll split that out and it can be rebased on top of bug 1073238 at some stage.

For future reference: Talked with Matt more about this on IRC, and we're thinking that generally bug 988151 is generally a better approach. If a page can rely on that, it can handle opening/closing menus itself - rather than UITour trying to be clever. That would let us remove a LOT of complexity in UITour.jsm.

Let this be known as the bug that resulted in many failed approaches. * Normal highlight panel won't show ontop of the searchbar dropwn, because <menupopup>s are top-most. Could make it level=top, but then that causes all sorts of other issues. * Tried to get the normal highlight style via a psuedo-element, because <menuitem>s are drawn with native styling. Messes up layout. * Finally settled on faking making the menuitem look like it's been hovered over. Bit of a hack, but at least it works.

(In reply to Blair McBride [:Unfocused] from comment #10) > Let this be known as the bug that resulted in many failed approaches. > * Normal highlight panel won't show ontop of the searchbar dropwn, because > <menupopup>s are top-most. Could make it level=top, but then that causes all > sorts of other issues. How about drawing a highlight to the side, such as an arrow? You'd probably need to worry about RTL (and drawing the arrow on the other side, if the panel to too close to the screen edge), but that doesn't sound too hard(tm)...

Comment on attachment 8503071 [details] [diff] [review] Patch v1 Review of attachment 8503071 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the missing returns and showHighlight fixed. ::: browser/modules/UITour.jsm @@ +831,5 @@ > showHighlight: function(aTarget, aEffect = "none") { > + function showHighlightPanel() { > + if (aTarget.targetName.startsWith(TARGET_SEARCHENGINE_PREFIX)) { > + aTarget.node.setAttribute("_moz-menuactive", true); > + return; You should also be hiding any existing highlight (including other engine highlights) for consistency. A test for that would probably be good. @@ +1075,5 @@ > + let menuBtn = aWindow.document.getElementById("bookmarks-menu-button"); > + openMenuButton(menuBtn); > + } else if (aMenuName == "searchEngines") { > + let menuBtn = this.targets.get("searchProvider").query(aWindow.document); > + openMenuButton(menuBtn); It seems like we could have used getTarget here as I don't think this needs to be synchronous especially since there is a callback. @@ +1321,5 @@ > + Task.spawn(function*() { > + let searchTarget = yield this.getTarget(aWindow, "search"); > + // We're not supporting having the searchbar in the app-menu, because > + // popups within popups gets crazy. > + if (!searchTarget.node || this.targetIsInAppMenu(searchTarget)) I personally wouldn't put this targetIsInAppMenu restriction in anymore now that we're using this different approach as the UX is no worse than a user manually opening the engine dropdown themeselves. @@ +1322,5 @@ > + let searchTarget = yield this.getTarget(aWindow, "search"); > + // We're not supporting having the searchbar in the app-menu, because > + // popups within popups gets crazy. > + if (!searchTarget.node || this.targetIsInAppMenu(searchTarget)) > + reject("Search engine not available"); You're missing the return and don't forget to add braces. @@ +1333,5 @@ > + if (engine && engine.identifier == aIdentifier) { > + resolve({ > + targetName: TARGET_SEARCHENGINE_PREFIX + engine.identifier, > + node: engineNode, > + }); Aren't you missing a return here too?

(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #13) > I personally wouldn't put this targetIsInAppMenu restriction in anymore now > that we're using this different approach as the UX is no worse than a user > manually opening the engine dropdown themeselves. We'd need either to add more magic to open the appmenu (already proven that ends up being too complex), or to wait until bug 988151 is implemented so the page knows to open the appmenu. Otherwise, we end up with the page thinking it can highlight a search engine, and it may or may not work. At least with this check in place, the page knows the highlight won't show and can do something else.

(In reply to Justin Dolske [:Dolske] from comment #12) > How about drawing a highlight to the side, such as an arrow? You'd probably > need to worry about RTL (and drawing the arrow on the other side, if the > panel to too close to the screen edge), but that doesn't sound too > hard(tm)... I don't think there's a way we can do that reliably, given: * We can't anchor a panel to an element in a <menupopup> to have it automatically positioned, due to a platform limitation * We can't reliably get the position of a popup, due to Linux

(In reply to Blair McBride [:Unfocused] from comment #15) > * We can't reliably get the position of a popup, due to Linux Uhh… this is going to be a problem for bug 1080944.

Comment on attachment 8503071 [details] [diff] [review] Patch v1 a+ for aurora/beta -- isolated impact, and want uplift since this will be going live with the anniversary release on 11/9.

https://hg.mozilla.org/releases/mozilla-aurora/rev/8cd0eedecef2 https://hg.mozilla.org/releases/mozilla-beta/rev/9d4b08eecd9a

TM tracks when this landed on trunk. I've added a tracking-firefox33 request based on comment 19.

Landed in alder: https://hg.mozilla.org/projects/alder/rev/4dc2af2e837c Based on https://hg.mozilla.org/mozilla-central/rev/ce98fd405802 from comment 18. Only noteworthy change was that Firefox 33 doesn't support the new shorthand JS syntax used for the implementations of getAvailableSearchEngineTargets() and getSearchEngineTarget(). Just used the equivalent "label: function(args) { ... }" syntax instead, as is used everywhere else. Caught with a test, so yay tests. :)

When testing this implementation via the tour I see a couple of issues: 1.) Calling showMenu('searchEngines') and highlighting a provider moves focus away from the web page. I can see the rationale here, but this has the side-effect that the user must click back onto the web page to regain focus, before they can then click "Next" to proceed to the next step in the tour. It means the user must effectively click "Next" twice to move on a step. This does not happen in contrast to highlighting other parts of the UI like we have done previously. 2.) If the search bar has customized and moved within "appMenu", calling showMenu('searchEngines') does nothing. 3.) When highlighting a search provider within the drop-down list, it does not seem to use the same consistent highlight style seen elsewhere. This might be for good reason, but thought I should flag it here.

Attached an image of highlight style seen when applied to "Twitter" as an example.

(In reply to Alex Gibson [:agibson] from comment #24) > Created attachment 8507846 [details] > search.png > > Attached an image of highlight style seen when applied to "Twitter" as an > example. Thanks for making progress on this and pointing out the highlight style. To stay consistent with the other highlight states we use, it's best if it can take on the circle highlight style shown in the bug description (and comment 5).

I think my main (perhaps even more important than the highlight style?), is the focus issue outlined in point 1. I wonder how many people would click "Next" and then think they we're at the end of the tour because nothing happens, without realizing it was a window focus issue.

(In reply to Alex Gibson [:agibson] from comment #26) > I think my main (perhaps even more important than the highlight style?), is > the focus issue outlined in point 1. I wonder how many people would click > "Next" and then think they we're at the end of the tour because nothing > happens, without realizing it was a window focus issue. Right. Definitely agree. This works differently than when the menu panel or doorhangers are expanded. The user should not have to click back into the web to regain focus, since this will feel like they are clicking "next" twice before the button works for them. We need to be consistent with interaction across all open panels and doorhangers. Do you think there is a way around this?

(In reply to Holly Habstritt Gaal [:Habber] from comment #27) > Do you think there is a way around this? Not via the web page (it loses all control when focus is lost). I think the only way to fix this is within Firefox. If this can't be done in time, the only thing I can think of would be to simplify this step by just calling Mozilla.UITour.showHighlight('searchProvider'), instead of opening the menu and highlighting an individual provider.

(In reply to Alex Gibson [:agibson] from comment #23) > When testing this implementation via the tour I see a couple of issues: > > 1.) Calling showMenu('searchEngines') and highlighting a provider moves > focus away from the web page. I can see the rationale here, but this has the > side-effect that the user must click back onto the web page to regain focus Hmm. That's not good. We could look at fixing that, although the current highlight is a bit of a hack, and moving focus around can be dicey. > 2.) If the search bar has customized and moved within "appMenu", calling > showMenu('searchEngines') does nothing. See comment 14. Doesn't feel like this reached full resolution, though. Blair, what's the page expected to do here? > 3.) When highlighting a search provider within the drop-down list, it does > not seem to use the same consistent highlight style seen elsewhere. This > might be for good reason, but thought I should flag it here. Yes, see previous discussion in this bug. Notably comments 10 / 12 / 15. The different highlight was a workaround for that. I wonder, given the focus issues in #1, if we should revisit this.

(In reply to Alex Gibson [:agibson] from comment #28) > If this can't be done in time, the > only thing I can think of would be to simplify this step by just calling > Mozilla.UITour.showHighlight('searchProvider'), instead of opening the menu > and highlighting an individual provider. Yeah. If we can't make the highlighting work acceptably, the fallback / Plan B would be to only hilight the default provider widget, and have the tour itself show/explain (eg via a screenshot) that DDG is in the list. Sort of a "hey, we added DDG to our defaults, click that thing to get to it". Not ideal, but seems workable.

(In reply to Justin Dolske [:Dolske] from comment #30) > (In reply to Alex Gibson [:agibson] from comment #28) > > > If this can't be done in time, the > > only thing I can think of would be to simplify this step by just calling > > Mozilla.UITour.showHighlight('searchProvider'), instead of opening the menu > > and highlighting an individual provider. > > Yeah. If we can't make the highlighting work acceptably, the fallback / Plan > B would be to only hilight the default provider widget, and have the tour > itself show/explain (eg via a screenshot) that DDG is in the list. Sort of a > "hey, we added DDG to our defaults, click that thing to get to it". Not > ideal, but seems workable. Yeah, showHighlight or showInfo.

The focus switching is, sadly, a platform limitation due to the search dropdown being used as a real menu, rather than just us manually opening a panel (the highlight being different is unrelated - it's not doing anything with focus, opening the menu is). If this is a problem, I'm surprised it's only been noticed now - because opening the bookmarks menu results in the same focus behaviour. That said, I don't actually understand *why* this is a problem for the page. Points 1 and 2 from comment 23 are intentional. Point 1 is due to a limitation on how much magic we can bake in without it getting crazy and unworkable - once we have bug 988151, the page will be able to know to open the appMenu itself (IMO, this is a tradeoff we have to make - the complexity of fixing it either way isn't worth it right now). Point 2 is due to platform limitations - the best available option we had. (In reply to Alex Gibson [:agibson] from comment #28) > I think the > only way to fix this is within Firefox. If this can't be done in time, the > only thing I can think of would be to simplify this step by just calling > Mozilla.UITour.showHighlight('searchProvider'), instead of opening the menu > and highlighting an individual provider. IMO, this is what should have been pursued at the start, when all this was found to be so complex. Unfortunately, there was such a push to follow the provided spec.

(In reply to Blair McBride [:Unfocused] from comment #32) > The focus switching is, sadly, a platform limitation due to the search > dropdown being used as a real menu, rather than just us manually opening a > panel (the highlight being different is unrelated - it's not doing anything > with focus, opening the menu is). If this is a problem, I'm surprised it's > only been noticed now - because opening the bookmarks menu results in the > same focus behaviour. > > That said, I don't actually understand *why* this is a problem for the page. The issue it creates is that when the drop down opens, the user must click twice to go to the next step. Because the first click just refocuses the web page, it feels like clicking "Next" does nothing. I think this might confuse a lot of folk, as this does not happen when highlighting in other steps. As for the same thing happening with the bookmarks menu, I don't think we have used that in a tour before either, which is why it's likely not been picked up on until now. > IMO, this is what should have been pursued at the start, when all this was > found to be so complex. Unfortunately, there was such a push to follow the > provided spec. Holly, given the comments how would you like to proceed here? In the time we have and the possible difficulties outlined above in fixing this, should we consider simplifying this to showing the highlight on 'searchProvider'? I'm attaching a screenshot of how this looks currently for reference. I think it is our next best solution for this step.

(In reply to Alex Gibson [:agibson] from comment #33) > The issue it creates is that when the drop down opens, the user must click > twice to go to the next step. Because the first click just refocuses the web > page, it feels like clicking "Next" does nothing. I think this might confuse > a lot of folk, as this does not happen when highlighting in other steps. Ah, presumably this is an OSX-only issue then, which limits it's scope quite substantially.

(In reply to Blair McBride [:Unfocused] from comment #34) > Ah, presumably this is an OSX-only issue then, which limits it's scope quite > substantially. In this case I wonder if we could just do Mozilla.UITour.showHighlight('searchProvider') for OSX users? (depending on what Holly thinks).

(In reply to Alex Gibson [:agibson] from comment #35) > (In reply to Blair McBride [:Unfocused] from comment #34) > > Ah, presumably this is an OSX-only issue then, which limits it's scope quite > > substantially. > > In this case I wonder if we could just do > Mozilla.UITour.showHighlight('searchProvider') for OSX users? (depending on > what Holly thinks). The focusing issue is really unfortunate, but I think it is more important to point out the specific search provider here. For example, highlighting Google when we are talking about another search provider in the content is not appropriate. Despite the focusing issue, I think we should move forward opening the menu and highlighting the provider of our choice in the non-circle style for osx users.

(In reply to Justin Dolske [:Dolske] from comment #29) Tying up one loose end... > > 2.) If the search bar has customized and moved within "appMenu", calling > > showMenu('searchEngines') does nothing. > > See comment 14. Doesn't feel like this reached full resolution, though. > Blair, what's the page expected to do here? After talking this over, I think I misunderstood how this was expected to work. Maybe it was obvious to everyone else. :) My (mis)understanding was that it sounded like the tour page was unable to determine when the search bar was in the appmenu, and thus was unable to know if it should try to highlight a specific engine or just generically highlight the search bar (via the "searchProvider" target). The answer is that the tour page should look for "searchEngine-foo" (where "foo" is the desired engine) in the results from getAvailableTargets(). When the search bar is in the appmenu, the searchEngine-* targets are unavailable. And so the page can know to fallback to the searchProvider target (unless there's no searchProvider target either, which means the user has entirely removed the search box from their UI). AFAIK the only edge-case left over is when the user has removed the "foo" engine from the browser, but left the searchbox with other engines in the UI. Thus searchEngine-foo won't be in the available targets list, and it would be a little weird for the tour to still try to indirectly promote it via searchProvider. This case probably isn't worth worrying about. Although I suppose it could still be detected indirectly: when the target list contains "searchProvider" and contains at least one other target matching "searchEngine-*". tl;dr: shouldn't need to do anything more in-browser for this.

> When the search bar is in the appmenu, the searchEngine-* targets are unavailable. And so the page can know to fallback to the searchProvider target (unless there's no searchProvider target either, which means the user has entirely removed the search box from their UI). Ah, this wasn't clear to me in testing - I was checking for the availability of both `searchProvider` and `searchEngine-*`, but didn't realize the latter would be unavailable if the searchbar was in the appMenu. This fallback solution makes sense i think, thanks.