Fix UITour highlighting of search

RESOLVED FIXED in Firefox 34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: benjamin, Assigned: mossop)

Tracking

unspecified
Firefox 36
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

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

Details

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

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

Comment 1

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

Updated

5 years ago
Attachment #8526137 - Attachment description: patch → beta patch
Posted 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: 5 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.