Closed Bug 1101669 Opened 10 years ago Closed 9 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 36
Tracking Status
firefox34 + verified
firefox35 + fixed
firefox36 + fixed

People

(Reporter: benjamin, Assigned: Dolske)

References

Details

Attachments

(3 files, 4 obsolete files)

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.
Attached file 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);
"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.
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?
Flags: needinfo?(philipp)
(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
Flags: needinfo?(philipp)
Group: mozilla-employee-confidential
(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.
Attached patch Patch v.WTF (obsolete) — Splinter Review
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.
Attached image . I <3 XUL (obsolete) —
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.
Attached patch Patch v.1 (obsolete) — Splinter Review
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.
Assignee: nobody → dolske
Attachment #8525746 - Attachment is obsolete: true
Attachment #8525747 - Attachment is obsolete: true
Attachment #8525789 - Flags: review?(jaws)
Is there a bug # for "Flare"?
Flags: needinfo?(benjamin)
Attached image Screenshot
Screenshot. (I tested by loading the already-listed firstrun page, and then calling showInfo() via the web console.)
Blocks: fx-UITour
Bug 1088660 is Flare (new search UI).
Flags: needinfo?(benjamin)
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.
Attachment #8525789 - Flags: review?(jaws) → review+
Patch v.1 doesn't seem to apply cleanly to beta for some reason, so here's the trivial fixup.
Attachment #8525428 - Attachment is obsolete: true
Attachment #8525789 - Attachment is obsolete: true
Comment on attachment 8527900 [details] [diff] [review]
Patch (applies to Beta)

[Triage Comment]
Attachment #8527900 - Flags: approval-mozilla-beta+
Attachment #8527900 - Flags: approval-mozilla-aurora+
Verified as fixed using:

FF 43.RC1
Build Id:20141125180439
OS: Win7 x64, Ubuntu 14.04 x64, Mac Os X 10.9.5
https://hg.mozilla.org/mozilla-central/rev/837b0643d0d6
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Target Milestone: Firefox 37 → Firefox 36
You need to log in before you can comment on or make changes to this bug.