Closed Bug 1101999 Opened 10 years ago Closed 9 years ago

Fix UITour highlighting of search

Categories

(Firefox :: Search, defect)

x86
macOS
defect
Not set
normal

Tracking

()

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

People

(Reporter: benjamin, Assigned: mossop)

References

Details

Attachments

(2 files, 2 obsolete files)

Currently the UITour highlights the search provider icon. This breaks with bug 1088660, and we also want the ability to highlight additional elements. This patch fixes the highlight (to highlight the magnifying glass) and also lets the tour highlight the "Change Search Settings" link.
Attached patch bug1101999 (obsolete) — Splinter Review
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
Attachment #8525698 - Flags: review?(florian)
Comment on attachment 8525698 [details] [diff] [review]
bug1101999

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

::: browser/modules/UITour.jsm
@@ +130,5 @@
>          let searchbar = aDocument.getElementById("searchbar");
> +        if (searchbar.hasAttribute("oneoffui")) {
> +          return aDocument.getAnonymousElementByAttribute(searchbar,
> +                                                          "anonid",
> +                                                          "searchbar-search-button");

Not completely sure what the UITour uses this for, but getting the magnifying glass icon when requesting something called "searchProvider" may be misleading.

@@ +143,5 @@
> +       query: (aDocument) => {
> +         let searchbar = aDocument.getElementById("searchbar");
> +         let element = aDocument.getAnonymousElementByAttribute(searchbar,
> +                                                                "anonid",
> +                                                                "open-engine-manager");

It seems to me that with this code you'll only get the "Manager Search Engines…" menuitem of the old UI. This node still exists in the binding of the new UI, but it's never going to be visible (and I'll remove it once we can remove the old UI).

The "Change Search Settings" button has the anonid search-settings and is part of the browser-search-autocomplete-result-popup binding in urlbarBindings.xml
Attachment #8525698 - Flags: review?(florian) → review-
Attached patch patch (obsolete) — Splinter Review
This adds a new target, searchIcon for the magnifying glass and updates the searchPrefsLink target to work right. It doesn't do anything about showing the popup but since it didn't in the past I assume that is fine.

With the new UI the ability to show the searchEngines menu is broken. I haven't stripped the code but I have commented out the test. I'm assuming this may get fixed/removed in bug 1101670.
Assignee: benjamin → dtownsend+bugmail
Attachment #8525698 - Attachment is obsolete: true
Attachment #8526136 - Flags: review?(florian)
Attached patch beta patchSplinter Review
Attachment #8526136 - Attachment is obsolete: true
Attachment #8526136 - Flags: review?(florian)
Attachment #8526137 - Flags: review?(florian)
Comment on attachment 8526137 [details] [diff] [review]
beta patch

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

This seems reasonable given what I know of the search UI. I don't know much about UITour.

::: browser/modules/test/browser_UITour.js
@@ +194,5 @@
>  
>      gContentAPI.showHighlight("urlbar");
>      waitForElementToBeVisible(highlight, checkDefaultEffect, "Highlight should be shown after showHighlight()");
>    },
> +  /*function test_highlight_search_engine(done) {

If you comment out this code instead of removing it, I should you should include a comment explaining why it's commented out.
Attachment #8526137 - Flags: review?(florian) → review+
(In reply to Florian Quèze [:florian] [:flo] from comment #5)

> If you comment out this code instead of removing it, I should you should

This is supposed to read "I think you should".
Turns out that test was disabled by the searchbar patch anyway so I just left it alone.

Landed on beta: https://hg.mozilla.org/releases/mozilla-beta/rev/adbcf6cbc6ff
Attachment #8526137 - Attachment description: patch → beta patch
Attached patch trunk patchSplinter Review
This applies to trunk
Comment on attachment 8526137 [details] [diff] [review]
beta patch

This landed on beta with a=gavin.
Attachment #8526137 - Flags: approval-mozilla-beta?
Attachment #8526137 - Flags: approval-mozilla-aurora?
Comment on attachment 8526137 [details] [diff] [review]
beta patch

Approving for Aurora since this has already landed on Beta, and it's a holiday so this is the last aurora approval round before the weekend (and merge-day).
Attachment #8526137 - Flags: approval-mozilla-beta?
Attachment #8526137 - Flags: approval-mozilla-beta+
Attachment #8526137 - Flags: approval-mozilla-aurora?
Attachment #8526137 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/c922c2fb6cf8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.