1101669 - UITour: showInfo("search") should drop its notification from the end of the search box, not the middle

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Benjamin Smedberg]

Attachment: Screen Shot 2014-11-19 at 12.44.11 PM.png

We'd like the UITour to be able to show notifications anchored on the search box. Currently this anchors on the middle of the search box. Screenshot and sample code attached.

Comment 1

[Benjamin Smedberg]

Attachment: Testcase HTML

To use this, you have to whitelist the domain you're using and set browser.uitour.requireSecure;false

Whitelisting snippet:
Services.perms.add(Services.io.newURI("http://localhost:8000", null, null), "uitour", 1);

Comment 2

[Justin Dolske [:Dolske]]

"End" of the search box? Why not the beginning? If the search bar is wide, it would seem odd to have it anchored way out in the middle of nowhere.

Comment 3

[Benjamin Smedberg]

Oh, I was attaching it mentally to the magnifying glass which is currently on the right but will be on the left. Philipp, what do we want?

Comment 4

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> Oh, I was attaching it mentally to the magnifying glass which is currently
> on the right but will be on the left. Philipp, what do we want?

Should be left, where the magnifying glass will be when Flare lands.
Should be aligned like this: http://cl.ly/image/071G0d473l0L

Comment 5

[Justin Dolske [:Dolske]]

(In reply to Philipp Sackl [:phlsa] from comment #4)

> Should be left, where the magnifying glass will be when Flare lands.
> Should be aligned like this: http://cl.ly/image/071G0d473l0L

Nitpick: I assume that's the case for LTR languages (like en-US, where the Yahoo change is happening), but for RTL the order is different.

So sounds like it should anchor to the "start" -- left in LTR, right in RTL.

Comment 6

[Justin Dolske [:Dolske]]

Attachment: Patch v.WTF

In theory this should be a fairly trivial patch, to just change the arrow panel's alignment from bottomcenter to bottomleft (ignoring RTL, and only doing this when target == search).

But of course that's too simple, because our arrow panel code seems to go braindead. The panel body is in the right place, but the arrow itself has now decided to place itself on the wrong side. O_o

I'll debug the panel code, but we probably will want a 34.x workaround that avoids touching that... Maybe special-case the searchbox anchor to be the engine icon/magnifying glass icon -- it'll be in the right place, and the panel alignment can stay as bottomcenter.

Comment 7

[Justin Dolske [:Dolske]]

Attachment: . I <3 XUL

Comment 8

[Justin Dolske [:Dolske]]

So...

adjustArrowPosition() in popup.xml isn't doing anything, because |this.alignmentPosition| is empty. And that appears to happen because GetAlignmentPosition() in nsMenuPopupFrame.cpp doesn't seem to know how to handle "bottomleft". The value of |position| at the top and bottom is -1 (UNKNOWN). If I switch the openPopup() call from "bottomleft topright" to just "after_start", the panel's arrow is at the right spot.

I'll file a followup on that. I think we can work around this well enough for now.

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

This seems to work. I also tested with the ForceRTL addon, the panel and offset seem to be automatically adjusted to the expected position as in LTR.

Probably would be good to check the magic 18 pixel offset on Windows, and with whatever comment 3/4 are talking about.

Comment 10

[Justin Dolske [:Dolske]]

Is there a bug # for "Flare"?

Comment 11

[Justin Dolske [:Dolske]]

Attachment: Screenshot

Screenshot. (I tested by loading the already-listed firstrun page, and then calling showInfo() via the web console.)

Comment 12

[Benjamin Smedberg]

Bug 1088660 is Flare (new search UI).

Comment 13

[:Gijs (he/him)]

Comment on attachment 8525789 [details] [diff] [review]
Patch v.1

Review of attachment 8525789 [details] [diff] [review]:
-----------------------------------------------------------------

r+ either way, but see below.

::: browser/modules/UITour.jsm
@@ +1073,5 @@
> +      // For the search field, which can be wide, point towards the
> +      // location where users would type or select an engine.
> +      if (aAnchor.targetName == "search") {
> +        alignment = "after_start";
> +        xOffset = 18;

I wonder if it wouldn't make more sense to override the anchor to be one of the anon kids of the search box (specifically, the search engine icon). The hardcoded 18px offset makes me nervous about styles and things like the devedition theme.

Comment 14

[Justin Dolske [:Dolske]]

Attachment: Patch (applies to Beta)

Patch v.1 doesn't seem to apply cleanly to beta for some reason, so here's the trivial fixup.

Comment 15

[Justin Dolske [:Dolske]]

Comment on attachment 8527900 [details] [diff] [review]
Patch (applies to Beta)

[Triage Comment]

Comment 16

[Justin Dolske [:Dolske]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5feb7c5bfa27

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/5feb7c5bfa27

Comment 18

[Catalin Varga [QA][:VarCat]]

Verified as fixed using:

FF 43.RC1
Build Id:20141125180439
OS: Win7 x64, Ubuntu 14.04 x64, Mac Os X 10.9.5

Comment 19

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/integration/fx-team/rev/837b0643d0d6

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/837b0643d0d6

Comment 21

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-beta/rev/fdb3e0b2cb18