1467469 - Firefox Data Collection Use hyperlinks are misaligned (Learn more) and checkboxes can be checked independent of being disabled

Status: NEW

(Firefox :: Settings UI, defect, P3)

Description

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

Attachment: Data Collection and Use misalign.PNG

[Affected versions]:
 Fx 60.0
 Fx 61.0b11
 Fx 62.0a2

[Affected platforms]:
 Windows 10 x64
 Windows 7 x64
 mac OS X 10.13
 Ubuntu 14.04 LTS

[Steps to reproduce]:
 1. Launch Firefox.
 2. Go to about:preferences#security.
 3. Scroll down until the "Firefox Data Collection Use" section.

[Expected result]:
 All the hyperlinks are aligned.

[Actual result]:
 The "Learn more" hyperlink from "Allow Firefox to send backlogged crash reports on your behalf" is not aligned accordingly.

[Regression range]:
 Will return with the regression range as soon as possible.

[Additional notes]:
 Please note the attachment.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

This was introduced by bug 1439930. Michael, can you try using textContent instead of setting the label via the attribute?

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Marking as regression in the sense that we had done work to not have right-aligned Learn More links before and these two snuck in.

Comment 3

[Francesco Lodolo [:flod]]

(removing Fluent from the subject, since it doesn't seem relevant based on comment 1)

Comment 4

[Osmose [:osmose, :mkelly]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> This was introduced by bug 1439930. Michael, can you try using textContent
> instead of setting the label via the attribute?

I'm not super-familiar with XUL, you mean setting the `textContent` attribute on the DOM node for the checkbox, instead of using the `data-l10n-id` attribute, right?

If so, when I do that, the text label of the checkbox doesn't change. If I set it on the <xul:label> inside the checkbox element, it sets the text correctly but doesn't fix the alignment of the Learn More links.

IIRC the problem is that the only way I could find to make the checkbox properly wrap long strings to avoid expanding the width of the entire preferences page was to flex them, which has the side effect of pushing the Learn More links to the right side.

I'll ask around for XUL help to see if I can find another solution.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Michael, can you keep the needinfo open until you find a solution for this?

Comment 6

[Osmose [:osmose, :mkelly]]

Attachment: Non-flexed checkbox labels

So after further testing, I've found that most of the checkboxes in about:preferences suffer from the same wrapping problems[1]. That we don't run into this (at least not in en-US) is simply luck due to string length.

After asking around #fx-team, it seems that this is a known limitation with <label>s and <checkbox>es in XUL without a great solution. We could solve this by using HTML instead of XUL, but we'd want to convert all the checkboxes in about:preferences at once, and would have to reimplement all <checkbox> behavior in HTML/JS/CSS, maybe with a WebComponent. Additionally, I ran into issues testing this idea out[2].

I think the best course of action is to choose one of the two "broken" states and defer fixing these specific checkboxes to the larger issue of rewriting the preferences frontend in HTML:

1. Keep the `flex="1"` on the <checkbox> elements, which keeps the "Learn More" links aligned to the right. Optionally, apply this flex to other checkboxes in the preferences. This will preserve the behavior of text being wrapped.

2. Remove `flex="1"` from the two checkboxes in question. One of them will extend the width of the page, causing the "jumping search box" behavior from bug 1439930.

jaws: What do you think? I couldn't find a bug for porting the preferences UI to HTML, and don't know if that's in the long-term roadmap.

[1] That is, their labels do not wrap, but instead extend the page width if they're too long. This causes the search box to jump around while performing searches depending on whether the extended label is present in the results (bug 1439930).

[2] When I tried adding an `<input type="checkbox" />` element to about:preferences, it was not displayed on the page, although it showed up in the DOM inspector. It didn't look like CSS was hiding it.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late to fix in 61/62 but we could still consider it in 63 or 64.

Comment 9

[Gingerbread Man]

(In reply to Cristian Comorasu [:ccomorasu], Release Desktop QA from comment #0)
>  The "Learn more" hyperlink from "Allow Firefox to send backlogged crash
> reports on your behalf" is not aligned accordingly.

The link for "Allow Nightly to send browser error reports (including error messages) to Mozilla" is now affected as well. Please undupe bug 1489544 if it should be kept separate.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking fix-optional for 64. We could still take a patch for 65, and if it's verified and doesn't seem risky, could still take fixes for 64 as well.

Comment 11

[Pascal Chevrel:pascalc]

Updating tracking flags as we get closer to the 64 release.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Happy to take a patch in nightly; if it seems low risk enough please feel free to request uplift to 65 beta.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1467469 - Allow for Firefox Data Collection descriptions to span multiple lines without expanding the preferences container wider and keeping the Learn More links adjacent to the description text as much as possible. r?jaws,flod

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1467469 - Disabled checkboxes should not accept label@control being clicked to toggle their checked state. r?Gijs

Comment 15

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me) from comment #14)

Created attachment 9051890 [details]
Bug 1467469 - Disabled checkboxes should not accept label@control being clicked to toggle their checked state. r?Gijs

I wonder if this is also the fix for bug 1532701. Pinging :mkaply to make sure he sees this.

Comment 17

[Mike Kaply [:mkaply]]

I wonder if this is also the fix for bug 1532701. Pinging :mkaply to make sure he sees this.

I don't think so. In their video, they were clicking on the box, not the label.

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:jaws, could you have a look please?

Comment 19

[:Gijs (he/him)]

This is because the patches don't actually completely work and we've been unable to figure out how to complete them.

Sylvestre, is there a way to tag bugs so the bot doesn't poke us about them?

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Yeah, the bot pings bugs only once!