Closed Bug 1548198 Opened 7 months ago Closed 7 months ago

Regression: QuantumBar - DropDown Results are 1px too far left

Categories

(Firefox :: Address Bar, defect, P3)

defect
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 68
Iteration:
68.4 - Apr 29 - May 12
Tracking Status
firefox68 --- verified

People

(Reporter: mehmet.sahin, Assigned: adw)

References

Details

(Keywords: regression)

Attachments

(8 files)

Nightly 68.0a1 (2019-04-30) (64-Bit)
macOS 10.14.4
MacBook Air NonRetina

STR:

1.) Enable QuantumBar
2.) Type something into the Address Bar
3.) Take a look at the results in the DropDown

Actual: The text in the DromDown is no longer aligned with the text in the Address Bar. The text in the DropDown seemed to be moved 1px too far to the left. (The icons are okay.)

Expected: The text in the DropDown should be moved 1px to the right to be aligned again with the Address Bar text.

This is a recent regression.

A screenshot is attached.

Thanks.

Here is the regression range:

Good: 2019-04-11-16-11-15-mozilla-central

Bad: 2019-04-12-09-34-00-mozilla-central

Maybe you can help to find the culprit changeset in this range?

What I noticed between the both builds is, that a fix for adjusting the alignment of the icons (moving by 1px to the left) has probably caused this actual issue with the text. Maybe this helps to find the changeset?

Many thanks :)

Thanks for reporting this. It's important polish, and it should be straightforward to fix, but it probably doesn't block quantumbar release.

Points: --- → 2
Priority: -- → P3

(In reply to Mehmet from comment #1)

What I noticed between the both builds is, that a fix for adjusting the
alignment of the icons (moving by 1px to the left) has probably caused this
actual issue with the text. Maybe this helps to find the changeset?

Can you link to the fix you're talking about? In that general timeframe, I see https://hg.mozilla.org/mozilla-central/rev/cec029155339 but when I reverted it, it didn't seem to fix the problem.

Flags: needinfo?(mehmet.sahin)

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

(In reply to Mehmet from comment #1)

What I noticed between the both builds is, that a fix for adjusting the
alignment of the icons (moving by 1px to the left) has probably caused this
actual issue with the text. Maybe this helps to find the changeset?

Can you link to the fix you're talking about? In that general timeframe, I see https://hg.mozilla.org/mozilla-central/rev/cec029155339 but when I reverted it, it didn't seem to fix the problem.

I will attach two screenshots, that is showing the difference.

In build 2019-04-11-16-11-15-mozilla-central the icons were 1px off and the text was okay. And in build 2019-04-12-09-34-00-mozilla-central the icons were okay and but text was 1px off.

So something before build 2019-04-11-16-11-15-mozilla-central must have broken the allignment of the icons. And between build 2019-04-11-16-11-15-mozilla-central and build 2019-04-12-09-34-00-mozilla-central there must be a change that has been corrected the alignment of the icons but has been broken the alignment of the text.

Screenshots are following in the next comments.

Flags: needinfo?(mehmet.sahin)

In quantumbar, favicons have 8px of margin-end:

https://searchfox.org/mozilla-central/rev/197210b8c139b64f642edaa3336d26b9c1761568/browser/themes/shared/urlbar-autocomplete.inc.css#9
https://searchfox.org/mozilla-central/rev/197210b8c139b64f642edaa3336d26b9c1761568/browser/themes/shared/urlbar-autocomplete.inc.css#100

But in awesomebar, it's 9px, e.g.: https://searchfox.org/mozilla-central/rev/197210b8c139b64f642edaa3336d26b9c1761568/toolkit/themes/osx/global/autocomplete.css#46

It's nice that quantumbar has the same amount of space between type-icon and favicon as it does between and favicon and the title, unlike awesomebar. i.e., there's a single margin-end value for both .urlbarView-type-icon and .urlbarView-favicon (urlbarViewIconMarginEnd, 8px). So instead of adding another 9px value for .urlbarView-favicon's margin-end, this patch removes the 1px of padding in the urlbar input. That has the added benefit that the space around the identity box and its search icon (shown when typing) is now more symmetrical, although there's still 1px more space on its right side than its left side.

This patch also adjusts spacing in the awesomebar popup so the alignment remains right there.

Assignee: nobody → adw
Status: NEW → ASSIGNED
Iteration: --- → 68.4 - Apr 29 - May 12
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c8ada1038c84
Remove 1px of padding in the urlbar input so that text in the popup is aligned with text in the input. r=dao
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Attached image Divider_Issue.png

Hey Drew,

Thanks for fixing this issue for the Quantum Bar.

I can confirm that it is fixed in latest Nightly for the Quantum Bar, many thanks for that.

But the strange thing is, that the text of the Input of the Awesome Bar (disabled_Quantum_Bar) also moved by 1px to the left so that it is no longer aligned with the Popup text now :) Was this intended?

A second thing is that the divider between Secure Chip and Input text no longer looks nicely centered. Maybe it could also move by 1px to the left to fix it. A screenshot is attached. WDYT?

Thanks in advance

Flags: needinfo?(atwilson)
Flags: needinfo?(atwilson) → needinfo?(adw)

(In reply to Mehmet from comment #10)

But the strange thing is, that the text of the Input of the Awesome Bar
(disabled_Quantum_Bar) also moved by 1px to the left so that it is no longer
aligned with the Popup text now :) Was this intended?

It looks fine to me. The text in the popup moved left but the text in the input also moved left. Could you post a screenshot of what you see?

Flags: needinfo?(adw)
Attached image Screenshot.png

Sure. Please find the screenshot attached.

Details:
browser.urlbar.quantumbar = false
68.0a1 (2019-05-09) (64-Bit)
macOS 10.14.4
MacBookAir Non-Retina

Hmm.. after looking again on it with another letter e.g. "A" it looks okay for me?!

Maybe some letters look different on the inputbar and in the dropdown?

(In reply to Mehmet from comment #13)

Maybe some letters look different on the inputbar and in the dropdown?

Yes, I see that too. I'm on a 2x screen though. The patch shouldn't have changed that and didn't afaict.

(In reply to Mehmet from comment #12)

Created attachment 9063886 [details]
Screenshot.png

Sure. Please find the screenshot attached.

If this is different from before the patch landed, please let me know.

Regressions: 1550584

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

If this is different from before the patch landed, please let me know.

Hi, I checked the build (2019-05-08-21-51-44-mozilla-central) before your patch landed and it was different there. The "h" was nicely aligned. Maybe another patch, which has been landed in the same range, is causing this? Since this is only affecting the Non_Quantum_Bar maybe this is not so important?

(BTW: Thanks for filing Bug 1550584.)

Thanks, I'll try to figure out what's going on.

(In reply to Mehmet from comment #12)

Created attachment 9063886 [details]
Screenshot.png

Sure. Please find the screenshot attached.

I checked the 2019-05-09-21-43-05 nightly (with the patch) in my Windows 10 VM at 1x, and I do see the same as your screenshot. But... I also see it in the 2019-05-08-21-51-44 nightly, where you said you didn't see it. On my Mac on the other hand, when I run at layout.css.devPixelsPerPx = 1, the alignment is right on both. :-/

So I don't know. It's really surprising that you're not seeing attachment 9063886 [details] on older nightlies too, because the patch removed exactly 1px from both the input and the popup text. Their alignment relative to each other shouldn't have changed.

Attached video screencast.mov

Maybe interesting for you: Looks like the main problem is the Dropdown. It seems that the row height's are different. The row height's of the Quantum Bar are smaller, where no issue happens. The row heights of the actual Awesome Bar are larger, where the issue appears.

Attached a screencast where I am switching between two screenshots: The one with the Google Logo in the first row is the Awesome Bar. The one without the Google Logo is the Quantum Bar.

fwiw, we accepted that QB rows may have a different height, provided the difference is uninteresting.
a 1 px disalignment in non-QB is also probably not a critical problem, considered QB is the default. It would become a problem only if we'd decide to disable the QB in the beta cycle (unlikely, even if not impossible). A minor problem, anyway.

(In reply to Marco Bonardo [::mak] from comment #20)

fwiw, we accepted that QB rows may have a different height, provided the difference is uninteresting.
a 1 px disalignment in non-QB is also probably not a critical problem, considered QB is the default. It would become a problem only if we'd decide to disable the QB in the beta cycle (unlikely, even if not impossible). A minor problem, anyway.

Okay, thanks.

Thanks. I do see the problem in your video. (I think you mean the one with the Google logo is quantumbar, though.)

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

(I think you mean the one with the Google logo is quantumbar, though.)

Ohh, yes, correct. Sorry :/

I just checked macOS 10.13 in a VM running at 1x, and with quantumbar disabled, the alignment is right in both before (2019-05-08-21-51-44) and after (2019-05-10-09-50-15) nightlies. So I really dunno.

Just to follow up with some additional info, Mehmet emailed me and said that the problem doesn't happen anymore, maybe because of toggling LCD font smoothing in system settings, but it's not clear. Also, the appearance of the letters in the popup is different depending on the width of the window, due to subpixel sampling looks like.

Flags: qe-verify-
Flags: qe-verify- → qe-verify+

Hi, This issue is Verified as Fixed in our latest Beta 68.0b13 on Windows Mac OsX and Ubuntu.

Status: RESOLVED → VERIFIED
Flags: qe-verify+

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression
You need to log in before you can comment on or make changes to this bug.