Last Comment Bug 720987 - Focus rings keep growing when repeatedly tabbing through elements
: Focus rings keep growing when repeatedly tabbing through elements
Status: RESOLVED FIXED
: regression, relnote
Product: Core
Classification: Components
Component: Layout: View Rendering (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla12
Assigned To: Mats Palmgren (:mats)
:
Mentors:
: 721030 722070 749450 (view as bug list)
Depends on: 722325
Blocks: 719177 524925 764554
  Show dependency treegraph
 
Reported: 2012-01-25 05:00 PST by Dão Gottwald [:dao]
Modified: 2014-03-16 22:32 PDT (History)
14 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
-
-


Attachments
testcase (266 bytes, text/html)
2012-01-25 05:00 PST, Dão Gottwald [:dao]
no flags Details
fix + test (4.21 KB, patch)
2012-01-25 21:00 PST, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review

Description Dão Gottwald [:dao] 2012-01-25 05:00:41 PST
Created attachment 591426 [details]
testcase
Comment 1 Dão Gottwald [:dao] 2012-01-25 05:03:46 PST
Probably caused by bug 524925 or bug 665597?
Comment 2 Jesse Ruderman 2012-01-25 09:41:57 PST
*** Bug 721030 has been marked as a duplicate of this bug. ***
Comment 3 Mats Palmgren (:mats) 2012-01-25 21:00:22 PST
Created attachment 591691 [details] [diff] [review]
fix + test

We shouldn't use the currently stored overflow areas for input to
FinishAndStoreOverflow since it expects the current frame rect +
child overflow to start from.  UpdateOverflow() does it right.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-25 21:22:56 PST
Comment on attachment 591691 [details] [diff] [review]
fix + test

Review of attachment 591691 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/bugs/720987.html
@@ +19,5 @@
> +  document.documentElement.removeAttribute("class");
> +}
> +</script>
> +</head>
> +<body style="padding:100px;" onload="setTimeout(doTest,0)">

Actually, this should use MozReftestInvalidate to make sure the document is rendered before doTest runs.

Hmm, do you have other tests doing this?
Comment 5 Mats Palmgren (:mats) 2012-01-27 19:53:22 PST
# grep -r 'onload="setTimeout' layout/reftests/ | wc -l
57

I guess some of those should probably use MozReftestInvalidate so I filed bug 721940
Comment 7 Mats Palmgren (:mats) 2012-01-28 16:32:28 PST
*** Bug 722070 has been marked as a duplicate of this bug. ***
Comment 8 Joe Drew (not getting mail) 2012-01-28 18:58:56 PST
https://hg.mozilla.org/mozilla-central/rev/731208933852
Comment 9 Chris Lord [:cwiiis] 2012-02-01 02:10:08 PST
This caused bug #722325, which made the native fennec browser unusable. To fix bug #722325, a partial revert was checked in (it reverts this change for transformed frames).

It seems that UpdateOverflow is incorrect somewhere with respect to CSS transforms?

Native fennec uses CSS translate + nsIFrameLoaderOwnder:clipSubdocument to buffer off-screen areas of an element, and this patch caused areas that should have been invalidated not to be invalidated.
Comment 10 Alex Keybl [:akeybl] 2012-02-10 12:32:43 PST
Tracking for FF12 - we'd definitely consider taking a for this during Aurora 12.
Comment 11 Alex Keybl [:akeybl] 2012-03-19 15:54:48 PDT
Mats - can you take a look at what caused the partialbackout?

Also adding qawanted to see if the partial revert caused this behavior to occur again.
Comment 12 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-26 19:00:38 PDT
(In reply to Alex Keybl [:akeybl] from comment #11)
> Mats - can you take a look at what caused the partialbackout?
> 
> Also adding qawanted to see if the partial revert caused this behavior to
> occur again.

How do we test a partial backout? Is this specific to Fennec?
Comment 13 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-27 14:38:15 PDT
Running the attached testcase in Firefox 11.0 and 12.0b2, the focus ring remains the same size. All I'm doing is loading the testcase and letting it run. Am I doing something wrong?
Comment 14 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-29 11:33:40 PDT
Bump. Please advise RE comment 13.
Comment 15 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-29 14:43:31 PDT
Retested this as per Jesse's instructions by testing in Nightly from 2012-01-25 then again in 12b3.

Nightly 12.0a1 2012-01-25 reproduces the bug; Beta 12.0b3 does not.
Comment 16 Alex Keybl [:akeybl] 2012-03-30 12:58:45 PDT
Thanks Anthony - we'll track for FF14 in that case.
Comment 17 Mats Palmgren (:mats) 2012-04-26 18:39:32 PDT
*** Bug 749450 has been marked as a duplicate of this bug. ***
Comment 18 Alex Keybl [:akeybl] 2012-04-30 15:08:26 PDT
We've seen new dupes of this issue - tracking for FF13.
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-01 12:27:02 PDT
At least in Firefox 13.0b1, I cannot reproduce the attached testcase. Is there anything QA can do?
Comment 20 Alex Keybl [:akeybl] 2012-05-14 09:03:31 PDT
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #19)
> At least in Firefox 13.0b1, I cannot reproduce the attached testcase. Is
> there anything QA can do?

If you're no longer able reproduce, no need to track for release. Would be good to find out if we can also untrack for FF14 at this point.
Comment 21 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-14 12:09:19 PDT
(In reply to Alex Keybl [:akeybl] from comment #20)
> If you're no longer able reproduce, no need to track for release. Would be
> good to find out if we can also untrack for FF14 at this point.

Strangely, in Firefox 14.0a2 2012-05-14, I don't get any focus rings (Win7 x64).
Comment 22 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-03 10:05:31 PDT
[Triage Comment]
I'm not sure if comment 21 is an affirmation that this can't be reproduced in 14, so flagging qawanted again just to confirm if we can untrack this for 14.
Comment 23 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-03 10:25:06 PDT
I repeated my testing. Firefox 14.0a2 2012-06-03 does not display any focus rings with the attached testcase. There's nothing more QA can do here without a different testcase or more specific instructions. I'm not exactly sure what you are looking for, Lukas.
Comment 24 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-03 11:00:02 PDT
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #23)
> I repeated my testing. Firefox 14.0a2 2012-06-03 does not display any focus
> rings with the attached testcase.

What I was looking for was a statement of whether not displaying focus rings was the desired result here. Is that equal to "does not reproduce"?  The bug summary claims focus rings keep growing and so it's not clear to me that no focus rings is the desired state and not another bug of 'no focus rings'. Does that make sense?
Comment 25 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-03 11:15:51 PDT
Here is what I am seeing, just to reiterate:
* Firefox 12.0b3: focus rings display but are the same size as the test goes on
* Firefox 12.0a1: focus rings display and they grow in size as the test goes on
* Firefox 14.0a2: focus rings do not display

The only conclusions I can make from this is that the fix is verified Firefox 12 and unverifiable in Firefox 14. In other words, does not reproduce != verified fixed. Sorry if that did not come across clearly in my previous comments. 

I'm not sure there is anything more QA can do here. Either the testcase is broken for Aurora or there is a bug in Aurora which needs to be resolved before we can verify this bug.

Dropping QAWANTED -- please re-add if there's something specific QA can do to help here.
Comment 26 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-20 16:44:45 PDT
Strangely, the focus rings are now appearing across all builds. I can no longer reproduce bug 764554 (see https://bugzilla.mozilla.org/show_bug.cgi?id=764554#c4). I think we can resolve that bug WORKSFORME. Additionally, I think that means we can stop tracking this bug; unless there is something more you want tested.
Comment 27 Alex Keybl [:akeybl] 2012-06-24 12:56:37 PDT
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #26)
> Strangely, the focus rings are now appearing across all builds. I can no
> longer reproduce bug 764554 (see
> https://bugzilla.mozilla.org/show_bug.cgi?id=764554#c4). I think we can
> resolve that bug WORKSFORME. Additionally, I think that means we can stop
> tracking this bug; unless there is something more you want tested.

Now that you're able to see the focus rings on 14, have you been able to reproduce the issue where focus rings grow when tabbing between elements?
Comment 28 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-24 14:35:07 PDT
No, as reported in comment 25, I've not been able to reproduce the focus ring growth on anything bug 12.0 Nightly. I've not been able to reproduce this issue on any Firefox 12 Aurora, Firefox 12 Beta, Firefox 13, 14, 15, or 16 builds. 

I don't see that there is anything left for QA to do here. If there are real-world users reporting this issue on anything other than 12.0 Nightly then I suggest the next action should be to reach out to them.

From my perspective, this is FIXED.
Comment 29 Alex Keybl [:akeybl] 2012-07-08 18:26:21 PDT
Thanks Anthony
Comment 30 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-08 21:59:37 PDT
The partial backout means this bug is not fixed for elements with transforms, but I believe bug 725664 covers that, so this bug should probably be marked fixed again.
Comment 31 flying sheep 2012-07-18 02:37:11 PDT
or we could just remove this unnecessary vendor element.

1. it causes unexpected behavior (additional padding)
2. it’s mentioned in no standard
3. it’s unnecessare in order to achieve the desired effect, a outline with negative offset will do fine

this would be bug 140562
Comment 32 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2014-03-16 22:32:10 PDT
This patch introduced the problem I describe in the paragraph beginning "We've also consistently had very odd handling" in bug 984226 comment 0.  I think it should be refixed as I describe in bug 984226 comment 3.

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