Bug 56401 - hover then un-hover makes state change
Summary: hover then un-hover makes state change
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.6
: P2 Normal
Assignee: Nobody
URL: http://idea.ed.ac.uk/bugs/css-before....
Keywords:
Depends on: 64854
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-15 12:35 PDT by Michael Fourman
Modified: 2011-08-02 10:08 PDT (History)
8 users (show)

See Also:


Attachments
Patch (4.94 KB, patch)
2011-06-20 01:04 PDT, Mihnea Ovidenie
morrita: review-
Details | Formatted Diff | Diff
Second patch (5.28 KB, patch)
2011-07-05 02:26 PDT, Mihnea Ovidenie
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.42 MB, application/zip)
2011-07-05 02:45 PDT, WebKit Review Bot
no flags Details
Patch 3 (5.52 KB, patch)
2011-07-05 04:28 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch 4 (5.63 KB, patch)
2011-07-06 00:40 PDT, Mihnea Ovidenie
hyatt: review-
hyatt: commit-queue-
Details | Formatted Diff | Diff
Patch 5 (5.58 KB, patch)
2011-07-14 07:23 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch 6 (5.62 KB, patch)
2011-07-19 09:29 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch 7 (9.60 KB, patch)
2011-07-27 09:53 PDT, Mihnea Ovidenie
hyatt: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch 8 (9.52 KB, patch)
2011-07-29 00:31 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch (9.50 KB, patch)
2011-08-01 10:18 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Fourman 2011-03-15 12:35:53 PDT
The document (see below) validates as HTML5.
Hovering then moving mouse away does not revert to original appearance.

Bug reproducible on the following browsers
Webkit Safari Nightly Version 5.0.4 (6533.20.27, r80833)
Google Chrome 10.0.648.133 beta
Safari release Version 5.0.4 (6533.20.27)

<!DOCTYPE html>
<html>
  <head>
    <meta charset="UTF-8"/>
    <title>A css bug?</title>
    <style type="text/css">
      td:before       { content:"> "; }
      td:hover:before { content:"< "; }
    </style>
  </head>
  <body>
<p>Surely the appearance should return to initial state when the hover is finished.</p>
    <table>
      <tr>
	<td>Hover over me to test then move the mouse away.
	  <ul></ul>
	</td>
      </tr>
    </table>
  </body>
</html>
Comment 1 Mihnea Ovidenie 2011-06-18 06:13:39 PDT
I have a fix for it.
Comment 2 Mihnea Ovidenie 2011-06-20 01:04:29 PDT
Created attachment 97761 [details]
Patch

Patch
Comment 3 Hajime Morrita 2011-07-04 23:15:17 PDT
Comment on attachment 97761 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97761&action=review

Thank you for doing this!
Unfortunately I'm not an expert in this area. So I'd like to have some prep review before calling the experts ;-)
That being said, the test strategy itself looks fine.

> LayoutTests/fast/dynamic/hover-before-position-after-style-change.html:1
> +<!DOCTYPE html>

We need <html>

> LayoutTests/fast/dynamic/hover-before-position-after-style-change.html:9
> +            function getElementTop(elementId) {

Webkit tests usually have one <script> block.
You can use layoutTestcontroller.waitUntilDone() and notifyDone() for keep DRT running during the exercise.

> LayoutTests/fast/dynamic/hover-before-position-after-style-change.html:26
> +            if (window.layoutTestController) {

Could you make this test runnable without layoutTestController?
WebKit devs prefer to keep tests runnable in Safari (or other browsers) as possible. 

An idiom is:
if (!window.layoutTestController)
    return;
inside a test function.


> Source/WebCore/ChangeLog:8
> +        https://bugs.webkit.org/show_bug.cgi?id=56401

Please follow the standard changelog format.
- You need a bug summary line before the URL line
- The detailed description should come after the URL.
Comment 4 Mihnea Ovidenie 2011-07-05 02:26:11 PDT
Created attachment 99686 [details]
Second patch

Thanks for the review. I have reworked the test to be used in browser too. I have also reworked the Changelog format.
Comment 5 WebKit Review Bot 2011-07-05 02:45:37 PDT
Comment on attachment 99686 [details]
Second patch

Attachment 99686 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8983569

New failing tests:
fast/dynamic/hover-before-position-after-style-change.html
Comment 6 WebKit Review Bot 2011-07-05 02:45:42 PDT
Created attachment 99687 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 7 Hajime Morrita 2011-07-05 03:19:52 PDT
Comment on attachment 99686 [details]
Second patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99686&action=review

> LayoutTests/fast/dynamic/hover-before-position-after-style-change-expected.txt:1
> +CONSOLE MESSAGE: line 0: 

You need to eliminate all error messages.

> LayoutTests/fast/dynamic/hover-before-position-after-style-change.html:1
> +<html>

Could you add doctype?

> LayoutTests/fast/dynamic/hover-before-position-after-style-change.html:10
> +            Hover the mouse over the "Inline" text. The test passes if you see a "PASS" below.

"PASS" is a kind of convention So you don't need to mention it explicitly.
Instead of that, manual testing instruction would be preferable here.

> LayoutTests/fast/dynamic/hover-before-position-after-style-change.html:20
> +            function getElementTop(elementId) {

"{" should be placed on the next line.
The coding convention is almost same as one of c++.
(Unfortunately, check-webkit-style doesn't check JavaScript inside HTML...)
Comment 8 Hajime Morrita 2011-07-05 03:20:29 PDT
Anyway, thank you for updating the patch quickly and I'm sorry for my absence at IRC.
Comment 9 Mihnea Ovidenie 2011-07-05 04:28:10 PDT
Created attachment 99698 [details]
Patch 3

Reworked patch based on suggestions.
Comment 10 Hajime Morrita 2011-07-05 18:47:57 PDT
Comment on attachment 99698 [details]
Patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=99698&action=review

> LayoutTests/fast/dynamic/hover-before-position-after-style-change.html:30
> +                    return;

This trick is a bit tricky. Could you make this more straightforward?
I guess we can split it into
- a function to emulate the mouse move
- another function to check the result.
Then |fromMouseOver| would be no longer required.
Comment 11 Hajime Morrita 2011-07-05 18:49:35 PDT
CC-ing expert to hear their opinion.
The change itself is just a few lines thus I hope it just OK.
Comment 12 Mihnea Ovidenie 2011-07-06 00:40:24 PDT
Created attachment 99800 [details]
Patch 4

Reworked test to make it clear.
Comment 13 Dave Hyatt 2011-07-13 11:37:21 PDT
Comment on attachment 99800 [details]
Patch 4

View in context: https://bugs.webkit.org/attachment.cgi?id=99800&action=review

> Source/WebCore/rendering/RenderObjectChildList.cpp:413
> +    if ((type == BEFORE) && insertBefore && insertBefore->isAnonymousBlock() && insertBefore->childrenInline()) {

Isn't (type == BEFORE) unnecessary in this if statement? insertBefore will be 0 if type is not == BEFORE.
Comment 14 Mihnea Ovidenie 2011-07-14 07:23:06 PDT
Created attachment 100805 [details]
Patch 5

Reworked patch.
Comment 15 Mihnea Ovidenie 2011-07-19 09:29:55 PDT
Created attachment 101332 [details]
Patch 6

Not sure why the patch failed in the first place.
Comment 16 Dave Hyatt 2011-07-19 11:53:24 PDT
Comment on attachment 101332 [details]
Patch 6

r=me
Comment 17 WebKit Review Bot 2011-07-19 12:25:43 PDT
Comment on attachment 101332 [details]
Patch 6

Clearing flags on attachment: 101332

Committed r91285: <http://trac.webkit.org/changeset/91285>
Comment 18 WebKit Review Bot 2011-07-19 12:25:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Ryosuke Niwa 2011-07-20 23:39:48 PDT
The patch was rolled out in http://trac.webkit.org/changeset/91349, re-opening the bug.
Comment 20 Mihnea Ovidenie 2011-07-27 01:28:42 PDT
The test that fails in debug is svg/custom/crash-textPath-attributes.html. An assertion: ASSERT(newNode->firstChild()); is hit in RenderCounter::makeCounterNode causing the test to fail.
Comment 21 Mihnea Ovidenie 2011-07-27 09:53:08 PDT
Created attachment 102154 [details]
Patch 7

Reworked the patch and also added a new test for the case i missed in the previous version of the patch.
Comment 22 Dave Hyatt 2011-07-28 11:38:29 PDT
Comment on attachment 102154 [details]
Patch 7

r=me
Comment 23 WebKit Review Bot 2011-07-28 16:26:44 PDT
Comment on attachment 102154 [details]
Patch 7

Rejecting attachment 102154 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2

Last 500 characters of output:
440d9e46bc8b9fc59df0eca9a6deb27770c211c8
r91953 = f8cbeecca54fbc917b123b223cc0e7057440dbb2
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/9264309
Comment 24 Mihnea Ovidenie 2011-07-29 00:31:43 PDT
Created attachment 102337 [details]
Patch 8

Same patch, hopefully the bot will be happier.
Comment 25 Mihnea Ovidenie 2011-08-01 10:18:39 PDT
Created attachment 102532 [details]
Patch
Comment 26 Dave Hyatt 2011-08-02 09:55:48 PDT
Comment on attachment 102532 [details]
Patch

r=me
Comment 27 WebKit Review Bot 2011-08-02 10:08:31 PDT
Comment on attachment 102532 [details]
Patch

Clearing flags on attachment: 102532

Committed r92200: <http://trac.webkit.org/changeset/92200>
Comment 28 WebKit Review Bot 2011-08-02 10:08:40 PDT
All reviewed patches have been landed.  Closing bug.