Closed Bug 1478604 Opened Last year Closed Last year

Focus visualization is not cleared when using border: 0 on an input element

Categories

(Core :: Layout: Form Controls, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- verified
firefox63 --- verified

People

(Reporter: PatrickWesterhoff, Assigned: emilio)

References

Details

(Keywords: regression)

Attachments

(3 files)

Attached file test-case.html
There seems to be some odd regression with the focus styling in Nightly. It seems that when focusing input elements using a label, the focus styling from a previously focused element is not cleared when that element has a `border: 0` styling.

Assume the following example (also attached):

    <label for="foo">Foo label</label>
    <input id="foo" type="text" value="foo" />
    <label for="bar">Bar label</label>
    <input id="bar" type="text" value="bar" />

    <style>
    input {
        border: 0;
    }
    </style>

If you click on the “Foo label”, then the `foo` element gets focused and its contents are selected, showing that blue selection background.

If you now click on the “Bar label”, you can see that the `bar` element gets focused and its contents are selected but at the same time, visually, the `foo` contents do not get deselected. It still looks as if the contents are selected.

If you start typing, you can see that the focus is indeed on the `bar` element. But the way it is, it looks as if there are multiple focus elements.


A colleague tried this on the Developer Edition 62.0b11 and could not reproduce this, so this seems to only apply to Nightly.
Attached image test-case-recording.gif
Here’s a quick demonstration of the problem.
Does seem to be a regression in 63.
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Ever confirmed: true
Has Regression Range: yes → ---
Keywords: regression
Just ran a mozregression test on this and I think I found the offending commit: https://hg.mozilla.org/integration/autoland/rev/a281dd360b0a

The last range the tool gave me was this: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ac9440a7256caa96064e9a7b259c89630a5aa50e&tochange=a281dd360b0a92a4eb0492a88170ddbf2a30ec74

I cannot make sense of the change but at least the commit message makes it sound related to this bug: “Don't process RepaintSelection() if the presshell is being destroyed.”
WFM on webrender, fwiw. That being said, that patch is wrong, and it's the likely cause of the bug. Thanks for the regression range, was super helpful!
Assignee: nobody → emilio
Blocks: 1438397
The regressing bug made RepaintSelectionRunner do nothing for any
nsISelectionListener that wasn't a PresShell.
Comment on attachment 8998161 [details]
Fix RepaintSelectionRunner so that it does something for non-presShell impls. r=masayuki,hiro

Masayuki Nakano [:masayuki] (JST, +0900) has approved the revision.

https://phabricator.services.mozilla.com/D2846
Attachment #8998161 - Flags: review+
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/8d41c10d3162
Fix RepaintSelectionRunner so that it does something for non-presShell impls. r=masayuki
https://hg.mozilla.org/mozilla-central/rev/8d41c10d3162
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment on attachment 8998161 [details]
Fix RepaintSelectionRunner so that it does something for non-presShell impls. r=masayuki,hiro

Approval Request Comment
[Feature/Bug causing the regression]: bug 1438397
[User impact if declined]: selection doesn't get repainted in some cases.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no (but should be trivial)
[Needs manual test from QE? If yes, steps to reproduce]: see comment 0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: trivial change that restores the behavior pre-bug 1438397.
[String changes made/needed]: none
Attachment #8998161 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Flags: in-testsuite+
I can confirm the fix in the latest Nightly V63.0a1 (2018-08-07) on Windows 10 x64 and Mac OS X 10.12.6.
The "foo" and "bar" editable content strings cannot be selected at once.

Confirmed fix in v63. Removing the QE-Verify flag.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
I can also confirm that this is fixed now. Thank you Emilio for the super quick fix!

And btw. I think it’s super awesome how you could create an automated test case for this that easily. That is really a very cool test system you have!
I am putting the QE-Verify+ flag back, at least until the uplift request is approved.
I have also confirmed the fix on Ubuntu 18.04 LTS .
Furthermore, I can confirm the reproduction on Beta version (v62.0b15).

Thank you.
Flags: qe-verify+
Comment on attachment 8998161 [details]
Fix RepaintSelectionRunner so that it does something for non-presShell impls. r=masayuki,hiro

fix a selection issue, verified in nightly, approved for 62.0b16
Attachment #8998161 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 1483661
Comment on attachment 8998161 [details]
Fix RepaintSelectionRunner so that it does something for non-presShell impls. r=masayuki,hiro

Actually I think we should fix this in ESR as well.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Trivial fix, annoying bug.
User impact if declined: See comment 0.
Fix Landed on Version: 63
Risk to taking this patch (and alternatives if risky): Very low risk patch.
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8998161 - Flags: approval-mozilla-esr60?
I have verified this fix in Beta v62.0b18; Focus visualization is now cleared correctly. Uplift successful. 
Marking Firefox62 as verified and removing qe-verify+ flag.
Flags: qe-verify+
Comment on attachment 8998161 [details]
Fix RepaintSelectionRunner so that it does something for non-presShell impls. r=masayuki,hiro

Simple fix which is more test than patch, approved for ESR 60.2.
Attachment #8998161 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.