Open Bug 660224 Opened 13 years ago Updated 2 years ago

Intermittent css-ui-invalid/default-style/input-focus.html, css-ui-invalid/default-style/textarea-focus.html | image comparison (==), max difference: 83, number of differing pixels: 1724 or 2448

Categories

(Core :: Layout: Form Controls, defect, P5)

Product:
Core
Component:
Layout: Form Controls
Type:
defect

Tracking

()

People

(Reporter: mounir, Unassigned)

References

Details

(Keywords: intermittent-failure, Whiteboard: [likely a real bug, unlikely a test flakyness][maybe an invalidation bug - see comment 268][test disabled on Windows][leave open])

Attachments

(1 file)

patch to make test use MozReftestInvalidate and MozAfterPaint
1.28 KB, patch
dholbert
: review+
Details | Diff | Splinter Review 
+++ This bug was initially created as a clone of Bug #626103 +++

These two failures are different from the select-*.html failures.
No longer blocks: 659708
No longer depends on: 626103
http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Beta/1306503277.1306507153.19612.gz
Rev3 WINNT 6.1 mozilla-beta debug test reftest on 2011/05/27 06:34:37
s: talos-r3-w7-020
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/layout/reftests/css-ui-invalid/default-style/textarea-focus.html | image comparison (==)
Whiteboard: [orange]
Mounir, looking at the images in this reftest, it seems like one of the screenshots has been taken before the CSS UI invalid highlight has finished animating.  Do we really paint these highlights as animations?
(In reply to comment #3)
> Mounir, looking at the images in this reftest, it seems like one of the
> screenshots has been taken before the CSS UI invalid highlight has finished
> animating.  Do we really paint these highlights as animations?

These highlights are simple box-shadow. I don't think box-shadow should be animated but I don't know how they internally work. Though, you might mistaken this because the box-shadow is lighter when the element has a focus ring so what you might think is an unfinished animation is just because the box-shadow didn't change.
See: https://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css#639
(In reply to comment #4)
> (In reply to comment #3)
> > Mounir, looking at the images in this reftest, it seems like one of the
> > screenshots has been taken before the CSS UI invalid highlight has finished
> > animating.  Do we really paint these highlights as animations?
> 
> These highlights are simple box-shadow. I don't think box-shadow should be
> animated but I don't know how they internally work. Though, you might
> mistaken this because the box-shadow is lighter when the element has a focus
> ring so what you might think is an unfinished animation is just because the
> box-shadow didn't change.
> See:
> https://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css#639

Hmm, so the first image has a crisp red border, while the second image has a lighter red border (which is probably because of the 0.4 alpha value).  Does that mean that the textarea was not focused when the first page was loaded?
Depends on: 660226
No longer depends on: 660226
(In reply to comment #6)
> Does that mean that the textarea was not focused when the first page was loaded?

The caret is inside the textarea so it's at least "somewhat" focused.
FWIW, this bug seems to only happen on Windows so far...
Summary: Intermittent failure in textarea-focus.html input-focus.html → Intermittent css-ui-invalid/default-style/input-focus.html, css-ui-invalid/default-style/textarea-focus.html | image comparison (==)
Whiteboard: [orange]
Summary: Intermittent css-ui-invalid/default-style/input-focus.html, css-ui-invalid/default-style/textarea-focus.html | image comparison (==) → Intermittent css-ui-invalid/default-style/input-focus.html, css-ui-invalid/default-style/textarea-focus.html | image comparison (==), max difference: 83, number of differing pixels: 1724 or 2448
IMO, this is showing a bug. The random orange disappeared for a 4 months and just came back...
Would that be possible that bug 539356 fixed this issue?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #268)
> Yes.

Any idea what could have regress the fix?
Marking this as depending on bug 808466. Hopefully, bug 808466 is the cause of this orange coming back.
Depends on: 808466
Whiteboard: [likely a real bug, unlikely a test flakyness]
(In reply to Mounir Lamouri (:mounir) from comment #266)
> IMO, this is showing a bug. The random orange disappeared for a 4 months and
> just came back...

(In reply to Mounir Lamouri (:mounir) from comment #282)
> Marking this as depending on bug 808466. Hopefully, bug 808466 is the cause
> of this orange coming back.

Bug 808466 didn't fix this. Any ideas?
Priority: -- → P5
Disabling on Windows for too many intermittent failures.
Whiteboard: [likely a real bug, unlikely a test flakyness] → [likely a real bug, unlikely a test flakyness][test disabled on Windows][leave open]
This bug has "likely a real bug, unlikely a test flakyness" on the whiteboard and we continue to hit it with pretty high frequency across all branches. Any change we could get someone to look into what the real bug might be here?
Flags: needinfo?(dholbert)
Flags: needinfo?(dbaron)
The rendering difference in comment 540 and comment 541 appears to be the difference between these two forms.css rules:

:not(output):-moz-ui-invalid {
  box-shadow: 0 0 1.5px 1px red;
}

:not(output):-moz-ui-invalid:-moz-focusring {
  box-shadow: 0 0 2px 2px rgba(255,0,0,0.4);
}


I find it interesting that this is (currently, at least) almost exclusively debug builds; the last occurrence on an opt build was comment 372.
I think it's worth testing whether adding a setTimeout(0) to the tests, in the path that removes reftest-wait from the focus event, makes this go away.
Flags: needinfo?(dbaron)
Perhaps jwatt can look?
Flags: needinfo?(jwatt)
[canceling my needinfo, since dbaron replied and it sounds like now jwatt might be looking into it]
Flags: needinfo?(dholbert)
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #543)
> The rendering difference in comment 540 and comment 541 appears to be the
> difference between these two forms.css rules:
> 
> :not(output):-moz-ui-invalid {
>   box-shadow: 0 0 1.5px 1px red;
> }
> 
> :not(output):-moz-ui-invalid:-moz-focusring {
>   box-shadow: 0 0 2px 2px rgba(255,0,0,0.4);
> }

FWIW, on Widows we draw _a_ focus ring when you hover a text box to match the platform defaults.  I'm not sure why but this keeps getting in the way of reftests.  I have fought this over and over again elsewhere (see bug 963763 for example) with disabling the native theming of text boxes with -moz-appearance: none.  That might be the right fix here too.
(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #548)
> FWIW, on Widows we draw _a_ focus ring when you hover a text box to match
> the platform defaults.  I'm not sure why but this keeps getting in the way
> of reftests.  I have fought this over and over again elsewhere (see bug
> 963763 for example) with disabling the native theming of text boxes with
> -moz-appearance: none.  That might be the right fix here too.

Are you saying that we match :-moz-focusring on hover (even without focus) on Windows?
Flags: needinfo?(ehsan)
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #549)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailapocalypse) from comment #548)
> > FWIW, on Widows we draw _a_ focus ring when you hover a text box to match
> > the platform defaults.  I'm not sure why but this keeps getting in the way
> > of reftests.  I have fought this over and over again elsewhere (see bug
> > 963763 for example) with disabling the native theming of text boxes with
> > -moz-appearance: none.  That might be the right fix here too.
> 
> Are you saying that we match :-moz-focusring on hover (even without focus)
> on Windows?

I haven't tested that so I'm not 100% sure, but we definitely match some kind of pseudo-selector there, as can easily be observed by hovering a textarea with the default style on windows.
Flags: needinfo?(ehsan)
I'll take a look. I would note that this line which sets |opacity: 0.54;| in forms.css looks suspicious given the difference in rendering during failure:

http://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css?rev=07dc7e2ed993&mark=171-171#152

Not sure how that could be applying to the border of the textarea rather than just its placeholder, though.
Flags: needinfo?(jwatt)
Looking at the ref now, I guess my previous comment is a red herring.
(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #550)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #549)
> > Are you saying that we match :-moz-focusring on hover (even without focus)
> > on Windows?
> 
> I haven't tested that so I'm not 100% sure, but we definitely match some
> kind of pseudo-selector there, as can easily be observed by hovering a
> textarea with the default style on windows.

That's just the state change on hover causing us to hit the NS_THEME_TEXTFIELD_MULTILINE case in nsNativeThemeWin::GetThemePartAndState which sets aState to TFS_EDITBORDER_HOVER. So I don't think that's relevant to this bug.
And even in the failures the textarea doesn't seem to have hover state (its top border would have a black line on the inner side instead of a dark grey line).
Whiteboard: [likely a real bug, unlikely a test flakyness][test disabled on Windows][leave open] → [likely a real bug, unlikely a test flakyness][maybe an invalidation bug - see comment 268][test disabled on Windows][leave open]
I haven't managed to reproduce this locally. I put the following block repeated 10000 times in a reftest.list file (so each test ran 10000 times) and didn't get any failures with Win 7 debug build build from a m-i pull from a few days ago:

  needs-focus == input-focus.html input-focus-ref.html
  needs-focus == button-focus.html button-focus-ref.html
  needs-focus == textarea-focus.html textarea-focus-ref.html
  needs-focus == select-focus.html select-focus-ref.html
Comment on attachment 8385407 [details] [diff] [review]
patch to make test use MozReftestInvalidate and MozAfterPaint

>+function startTest()
>+{
>+  window.addEventListener('MozAfterPaint', finishTest, false);
[...]
>+
>+function finishTest()
>+{
>+  document.documentElement.className='';
>+}
>+
>+window.addEventListener("MozReftestInvalidate", startTest, false);
>+
>   </script>
>-  <body onload="onloadHandler();">
>-    <textarea id='e' onfocus="document.documentElement.className='';"></textarea>

So just to be clear: this change means we won't be testing that 'onfocus' fires anymore (since we won't be using that event for snapshot-timing now). (Previously, we were, in this test.)

That's probably fine, I think; it looks like the bug that added this (bug 612752) was concerned with styling/rendering, not event-firing.

So: r=me
Attachment #8385407 - Flags: review?(dholbert) → review+
I've been seeing some very misleading behavior while debugging this. Using the patch from comment 597, modified to wait 30 seconds before finishing the test, it seemed like the test would pass every time the mouse pointer was over the browser window (regardless of which part of the window), and fail every time the pointer was outside the window. That seemed consistent for several dozen runs, but then suddenly the test started failing regardless of hover. I can't really explain that, it's just weird.
Flags: needinfo?(jwatt)
It seems the actual thing that's going on is that if the mouse is moving around the time that the top level windows open then the test "fails", but if it's not it "passes". (I think during the first series of test runs described in my previous comment my console was on the left of the screen, and I was moving it to check if it was over where the window was going to appear or not, whereas subsequently I had my console on the right of the screen and wasn't doing that because I knew the mouse was last out of the way of the test windows.) Fun, fun.
More progress...

The :-moz-focusring pseudo-class is breaking during the test failures.

This pseudo-class is only matched if the element state has NS_EVENT_STATE_FOCUSRING, which only happens if NotifyFocusStateChange is passed true for its aWindowShouldShowFocusRing parameter, which only happens if nsGlobalWindow::ShouldShowFocusRing() returns true, which, for this testcase on Windows, only happens if mShowFocusRings is true. mShowFocusRings is initialized to true, but then its value depends on whether the widget code calling nsGlobalWindow::SetKeyboardIndicators, and the value that gets passed, which depends on what nsWindow::ProcessMessage passes to NotifyUIStateChanged when it gets a UISF_HIDEFOCUS message. That message is dispatched by nsWindow::Show(), and perhaps can be dispatched by the OS.

In the case of running the tests under layout/reftests/css-ui-invalid/default-style we always get two UISF_HIDEFOCUS messages dispatched by nsWindow::Show, regardless of whether we pass or fail (one for each of the two toplevel windows when the open at the start of the test run). In the case that we "pass", nsWindow::ProcessMessage is never called with a UISF_HIDEFOCUS. In the case that we "fail", it is.

I really have no idea why moving the mouse around while the test harness opens the toplevel windows at the start of the test run would result in us [being more likely to] not receive the dispatched UISF_HIDEFOCUS events. Anyone got any ideas?

I'd also note that when we do get the UISF_HIDEFOCUS messages and call NotifyUIStateChanged the showFocusRings variable is true. That initially looks like it would _keep_ the focus ring state working, not break it, so some digging is needed to understand that part of the puzzle too.
The call stack that should result in nsWindow::ProcessMessage processing the UISF_HIDEFOCUS message should look like this:

  xul.dll!nsWindow::ProcessMessage
  xul.dll!nsWindow::WindowProcInternal
  xul.dll!CallWindowProcCrashProtected
  xul.dll!nsWindow::WindowProc            <--  not always called
  user32.dll!_InternalCallWinProc@20
  user32.dll!_UserCallWinProcCheckWow@32
  user32.dll!_RealDefWindowProcWorker@24
  user32.dll!_RealDefWindowProcW@16
  user32.dll!_DefWindowProcW@16
  user32.dll!_InternalCallWinProc@20
  user32.dll!_UserCallWinProcCheckWow@32
  user32.dll!_CallWindowProcAorW@24
  user32.dll!_CallWindowProcW@20          <--  always called
  xul.dll!nsWindow::WindowProcInternal
  xul.dll!CallWindowProcCrashProtected
  xul.dll!nsWindow::WindowProc
  user32.dll!_InternalCallWinProc@20
  user32.dll!_UserCallWinProcCheckWow@32
  user32.dll!_SendMessageWorker@24
  user32.dll!_SendMessageW@16
  xul.dll!nsWindow::Show

In the case of both success and failure we get to the CallWindowProcW at the end of nsWindow::WindowProcInternal during its outer call. The arguments passed to CallWindowProcW are always the same (the targetWindow->GetPrevWindowProc() call returns the same callback pointer, and presumably mWnd must be correct), and CallWindowProcW always returns 0. Yet somehow the resulting nsWindow::WindowProc call doesn't happen sometimes, depending on whether there have been mouse pointer moves or not.

I'm not sure how to tell what's different that changes what happens in the user32.dll code under the CallWindowProcW call.
Is the value of sIsInMouseCapture different in the two runs?
No, sIsInMouseCapture is false in both. I actually only need to move the mouse briefly right as the browser starts to make a difference, and it's a couple of seconds later that the nsWindow::Show code is hit.
CCing some folks who know more about Windows than I do...
Blocks: css-ui-3
This bug appears to be specific to a pseudo-class treatment, not any CSS-UI property.  As all selectors formerly in CSS-3 UI have been moved to a Selectors module draft, this does not block implementing CSS3-UI.

It appears that this is about :invalid but I can't be sure from reading the above, so I'm not sure which other metabug (if any) to actually block with this bug.
No longer blocks: css-ui-3
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: