Closed
Bug 1101999
Opened 10 years ago
Closed 9 years ago
Fix UITour highlighting of search
Categories
(Firefox :: Search, defect)
Tracking
()
RESOLVED
FIXED
Firefox 36
People
(Reporter: benjamin, Assigned: mossop)
References
Details
Attachments
(2 files, 2 obsolete files)
7.51 KB,
patch
|
florian
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
5.75 KB,
patch
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
Comment 2•10 years ago
|
||
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-
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8526136 -
Attachment is obsolete: true
Attachment #8526136 -
Flags: review?(florian)
Attachment #8526137 -
Flags: review?(florian)
Comment 5•10 years ago
|
||
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+
Comment 6•10 years ago
|
||
(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".
Assignee | ||
Comment 7•10 years ago
|
||
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
Updated•10 years ago
|
status-firefox34:
--- → fixed
status-firefox35:
--- → affected
status-firefox36:
--- → affected
tracking-firefox34:
--- → +
tracking-firefox35:
--- → +
tracking-firefox36:
--- → +
Assignee | ||
Updated•10 years ago
|
Attachment #8526137 -
Attachment description: patch → beta patch
Assignee | ||
Comment 8•10 years ago
|
||
This applies to trunk
Comment 9•9 years ago
|
||
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 10•9 years ago
|
||
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+
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/c922c2fb6cf8
Whiteboard: [fixed-in-fx-team]
Comment 12•9 years ago
|
||
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
Updated•9 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•