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

RESOLVED FIXED in Firefox 34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: benjamin, Assigned: Dolske)

Tracking

unspecified
Firefox 36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox34+ verified, firefox35+ fixed, firefox36+ fixed)

Details

Attachments

(3 attachments, 4 obsolete attachments)

Reporter

Description

5 years ago
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.
Reporter

Comment 1

5 years ago
Posted 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);
Assignee

Comment 2

5 years ago
"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.
Reporter

Comment 3

5 years ago
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
Assignee

Comment 5

5 years ago
(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.
Assignee

Comment 6

5 years ago
Posted 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.
Assignee

Comment 7

5 years ago
Posted image . I <3 XUL (obsolete) —
Assignee

Comment 8

5 years ago
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.
Assignee

Comment 9

5 years ago
Posted 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)
Assignee

Comment 10

5 years ago
Is there a bug # for "Flare"?
Flags: needinfo?(benjamin)
Assignee

Comment 11

5 years ago
Posted image Screenshot
Screenshot. (I tested by loading the already-listed firstrun page, and then calling showInfo() via the web console.)
Reporter

Comment 12

5 years ago
Bug 1088660 is Flare (new search UI).
Flags: needinfo?(benjamin)

Comment 13

5 years ago
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+
Assignee

Comment 14

5 years ago
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
Assignee

Comment 15

5 years ago
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
Last Resolved: 5 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.