Closed Bug 1567301 Opened 5 years ago Closed 5 years ago

title of awesomebar entry has grey background when tag is present

Categories

(Firefox :: Address Bar, defect, P1)

Product:
Firefox
Component:
Address Bar
Type:
defect
Points:
2

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 70
Iteration:
70.2 - Jul 22 - Aug 4
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- verified
firefox68 --- wontfix
firefox69 --- verified
firefox70 --- verified

People

(Reporter: jaws, Assigned: adw)

References

(Regression)

Details

(Keywords: regression)

Attachments

(5 files, 4 obsolete files)

2019-07-18_1247.png
141.74 KB, image/png
Details
Bug 1567301 - UnifiedComplete: Change the title-tags separator to a non-printable character.
47 bytes, text/x-phabricator-request
Details | Review
Beta/69 patch with fix for failing test
9.32 KB, patch
Details | Diff | Splinter Review
Beta/69 follow-up patch for eslint failure
1.18 KB, patch
Details | Diff | Splinter Review
Correct esr68 patch v2
9.41 KB, patch
Details | Diff | Splinter Review
Attached image 2019-07-18_1247.png —

Seen in 70.0a1 (2019-07-17) (64-bit) Nightly on Windows 10.

The title on the awesomebar entry is styled as a tag. Working on a regression range.

Priority: -- → P1

Confirmed via mozregression this was regressed by bug 1553067.

Flags: needinfo?(mak77)
Keywords: regressionwindow-wanted
Regressed by: 1553067

Not sure it matters yet, but the bookmark in the screenshot is tagged "first bug", see: https://www.screencast.com/t/rVuZMDv9hcU

Also, I notice that the bookmark shows up twice in the results... And Jared says it reproduces in his normal profile but not a new one.

STR (lol)

  1. Create any bookmark that uses an en dash ("–" \u2013) in its title, with some text following the en dash

https://searchfox.org/mozilla-central/rev/96403eac4ff6f96f89a0b120b29bd46540d5705e/toolkit/components/places/UnifiedComplete.jsm#24

Still trying to understand what bug 1553067 did to change this

(In reply to Drew Willcoxon :adw from comment #3)

Still trying to understand what bug 1553067 did to change this

In mozregression the builds before bug 1553067, the title stopped after the en-dash. I tried to find what caused that regression but the builds stopped working (no pages would load and the menus wouldn't appear).

Interesting, I'm guessing we never handled en dashes properly, or at least not for a while, and bug 1553067 ironically exposed it more because it fixed a bug in tag handling.

This just changes the TITLE_TAGS_SEPARATOR to the unprintable character \x1F, the unit separator, which seems appropriate.

At first I thought we could use the result label to store tags since we're not using the label at all right now. Hacky, but better than storing them in the title. But (1) nsAutoCompleteSimpleResult::GetLabelAt falls back to the value if the label is empty, and (2) nsAutoCompleteController::GetLabelAt actually returns the same thing as GetValueAt, i.e., the value, not the label. It's doable, but we'd need set the label to some special value for every result that doesn't have tags so that the label doesn't fall back to the value so we can tell which results don't have tags, and we'd need to make sure to always directly ask results for labels instead of going through the controller. head_autocomplete.js goes through the controller, and I didn't check what else does too.

So then I thought we could store tags in the style with a special substring like "tags=tag1,tag2,tag3". Again it's doable, but:

The simplest fix is to just change the separator to an unprintable character. That should work, right? We can do better whenever we finally rewrite/refactor UnifiedComplete for quantumbar.

Flags: needinfo?(mak77)
Assignee: nobody → adw
Status: NEW → ASSIGNED
Iteration: --- → 70.2 - Jul 22 - Aug 4
Points: --- → 2
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4d756df43d39
UnifiedComplete: Change the title-tags separator to a non-printable character. r=mak

Bug 1553067 is on 68, so this will need to be uplifted as far as reasonable.

status-firefox68: --- → affected
status-firefox69: --- → affected
status-firefox70: --- → affected
status-firefox-esr60: --- → unaffected
status-firefox-esr68: --- → affected

https://hg.mozilla.org/mozilla-central/rev/4d756df43d39

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox70: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70

STR

STR

  1. Open the bookmarks/history Library (Bookmarks > Show all Bookmarks)
  2. Select Other Bookmarks in the Library's sidebar
  3. Right-click in the main pane (where bookmarks are listed), choose New Bookmark
  4. Set the title to exactly: Title – This is not a tag
  5. Set the location to anything e.g. http://example.com/
  6. Set the tags to a tag name e.g. "tag" (without quotes)
  7. Type the tag name in the urlbar

Results without the fix: In the items in the urlbar popup, "This is not a tag" will have a gray background, i.e. it will be styled like a tag, and the dash in the title will be absent.

Expected results with the fix: "This is not a tag" should not have a gray background, it should be a normal part of the full title and the dash should be present, i.e. "Title – This is not a tag"

Flags: qe-verify+
Flags: in-testsuite-

(In reply to Drew Willcoxon :adw from comment #11)

Expected results with the fix: "This is not a tag" should not have a gray background, it should be a normal part of the full title and the dash should be present, i.e. "Title – This is not a tag"

Also, "tag" (or whatever you chose for the tag name) with a gray background should appear after the title, indicating that the item is tagged with "tag"

Attached patch Beta/69 patch (obsolete) — — Splinter Review

Beta/Release Uplift Approval Request

  • User impact if declined: The regression will remain on 69, i.e. users who have history/bookmarks that both (a) have an en dash in their titles and (b) are tagged will see incorrect items in the urlbar popup.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Please see comment 11 and 12
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Small patch that only changes how we internally separate titles and tags in urlbar results. This specific bug isn't covered by automated tests, but we have lots of tests around urlbar results. I've tested the patch manually on beta/69.
  • String changes made/needed: None
Attachment #9080094 - Flags: approval-mozilla-beta?
Attached patch esr68 patch (obsolete) — — Splinter Review

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Not critical, but a nice-to-fix on esr
  • User impact if declined: The regression will remain on esr68, i.e. users who have history/bookmarks that both (a) have an en dash in their titles and (b) are tagged will see incorrect items in the urlbar popup.
  • Fix Landed on Version: 70
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Small patch that only changes how we internally separate titles and tags in urlbar results. This specific bug isn't covered by automated tests, but we have lots of tests around urlbar results. I've tested the patch manually on esr68.
  • String or UUID changes made by this patch: None
Attachment #9080095 - Flags: approval-mozilla-esr68?
Comment on attachment 9080094 [details] [diff] [review]
Beta/69 patch

Fixes a visual regression in the awesomebar introduced in 68. Thanks for included rebased branch patches. Approved for 69.0b8 and 68.1esr.
Attachment #9080094 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9080095 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

The ESR68 patch doesn't apply for me. Can you please double-check that it's based on ESR68 tip?

Flags: needinfo?(adw)
Comment on attachment 9080095 [details] [diff] [review]
esr68 patch

Sorry, I must have forgotten to update my tree before importing the patch. To avoid confusion, I'll mark this as obsolete even though it has the approval+, and I'll post the correct patch and request approval again. Hope that's OK...
Attachment #9080095 - Attachment is obsolete: true
Flags: needinfo?(adw)
Attached patch Correct esr68 patch (obsolete) — — Splinter Review

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Not critical, but a nice-to-fix on esr
  • User impact if declined: The regression will remain on esr68, i.e. users who have history/bookmarks that both (a) have an en dash in their titles and (b) are tagged will see incorrect items in the urlbar popup.
  • Fix Landed on Version: 70
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Small patch that only changes how we internally separate titles and tags in urlbar results. This specific bug isn't covered by automated tests, but we have lots of tests around urlbar results. I've tested the patch manually on esr68.
  • String or UUID changes made by this patch: None
Attachment #9080160 - Flags: approval-mozilla-esr68?
Attachment #9080095 - Flags: approval-mozilla-esr68+
Comment on attachment 9080160 [details] [diff] [review]
Correct esr68 patch

No worries, thanks!
Attachment #9080160 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
QA Whiteboard: [qa-triaged]

I successfully reproduced the issue on Firefox Nightly 70.0a1 (2019-07-18) under Windows 10 (x64) using the STR from Comment 11.

The issue is no longer reproducible on latest Nightly 70.0a1 (2019-07-24). Tests were performed under WIndows 10 (x64), Ubuntu 18.04 (x64) and macOS 10.14.

status-firefox70: fixed → verified

This fixes the failure on beta/69 from comment 22.

Attachment #9080094 - Attachment is obsolete: true
Flags: needinfo?(adw)
Attachment #9080393 - Flags: checkin?(aryx.bugmail)

Sebastian, I'm sorry but there's going to be the same failure on esr68 as there was on beta, I just realized now. Could you back this out please? I'll post a new patch.

Flags: needinfo?(aryx.bugmail)

This fixes the (not yet reported) test failure on esr68.

Attachment #9080160 - Attachment is obsolete: true
Attachment #9080418 - Flags: checkin?(aryx.bugmail)
Comment on attachment 9080418 [details] [diff] [review]
Correct esr68 patch with fix for failing test

This has an eslint failure. Will post yet another patch.
Attachment #9080418 - Attachment is obsolete: true
Attachment #9080418 - Flags: checkin?(aryx.bugmail)

Follow-up beta/69 fix for eslint failure, sorry again

Attachment #9080423 - Flags: checkin?(aryx.bugmail)

... and the correct esr68 patch with fixes for the failing test and eslint.

Attachment #9080424 - Flags: checkin?(aryx.bugmail)

The issue is no longer reproducible on Firefox 69.0b8 (20190724202328 from Treeherder), and on Firefox 68.1.0esr (20190724210702 from Treeherder). Tests were performed under Windows 10 (x64), Ubuntu 18.04 (x64) and macOS 10.14.

Status: RESOLVED → VERIFIED
status-firefox69: fixed → verified
status-firefox-esr68: fixed → verified
Flags: qe-verify+
Regressions: 1579612
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: