Bug 56401

Summary:

hover then un-hover makes state change

Product:

WebKit

Reporter:

Michael Fourman <mpfourman>

Component:

CSS

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, hyatt, mihnea, mitz, morrita, rniwa, shanestephens, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac (Intel)

OS:

OS X 10.6

URL:

http://idea.ed.ac.uk/bugs/css-before.html

Bug Depends on:

64854

Bug Blocks:

Attachments:

Description	Flags
Patch	morrita: review-
Second patch	webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-03	none
Patch 3	none
Patch 4	hyatt: review-, hyatt: commit-queue-
Patch 5	none
Patch 6	none
Patch 7	hyatt: review+, webkit.review.bot: commit-queue-
Patch 8	none
Patch	none

Michael Fourman

Reported 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>

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
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Mihnea Ovidenie

Comment 1 2011-06-18 06:13:39 PDT

I have a fix for it.

Mihnea Ovidenie

Comment 2 2011-06-20 01:04:29 PDT

Created attachment 97761 [details] Patch Patch

Hajime Morrita

Comment 3 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.

Mihnea Ovidenie

Comment 4 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.

WebKit Review Bot

Comment 5 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

WebKit Review Bot

Comment 6 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

Hajime Morrita

Comment 7 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...)

Hajime Morrita

Comment 8 2011-07-05 03:20:29 PDT

Anyway, thank you for updating the patch quickly and I'm sorry for my absence at IRC.

Mihnea Ovidenie

Comment 9 2011-07-05 04:28:10 PDT

Created attachment 99698 [details] Patch 3 Reworked patch based on suggestions.

Hajime Morrita

Comment 10 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.

Hajime Morrita

Comment 11 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.

Mihnea Ovidenie

Comment 12 2011-07-06 00:40:24 PDT

Created attachment 99800 [details] Patch 4 Reworked test to make it clear.

Dave Hyatt

Comment 13 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.

Mihnea Ovidenie

Comment 14 2011-07-14 07:23:06 PDT

Created attachment 100805 [details] Patch 5 Reworked patch.

Mihnea Ovidenie

Comment 15 2011-07-19 09:29:55 PDT

Created attachment 101332 [details] Patch 6 Not sure why the patch failed in the first place.

Dave Hyatt

Comment 16 2011-07-19 11:53:24 PDT

Comment on attachment 101332 [details] Patch 6 r=me

WebKit Review Bot

Comment 17 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>

WebKit Review Bot

Comment 18 2011-07-19 12:25:51 PDT

All reviewed patches have been landed. Closing bug.

Ryosuke Niwa

Comment 19 2011-07-20 23:39:48 PDT

The patch was rolled out in http://trac.webkit.org/changeset/91349, re-opening the bug.

Mihnea Ovidenie

Comment 20 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.

Mihnea Ovidenie

Comment 21 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.

Dave Hyatt

Comment 22 2011-07-28 11:38:29 PDT

Comment on attachment 102154 [details] Patch 7 r=me

WebKit Review Bot

Comment 23 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

Mihnea Ovidenie

Comment 24 2011-07-29 00:31:43 PDT

Created attachment 102337 [details] Patch 8 Same patch, hopefully the bot will be happier.

Mihnea Ovidenie

Comment 25 2011-08-01 10:18:39 PDT

Created attachment 102532 [details] Patch

Dave Hyatt

Comment 26 2011-08-02 09:55:48 PDT

Comment on attachment 102532 [details] Patch r=me

WebKit Review Bot

Comment 27 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>

WebKit Review Bot

Comment 28 2011-08-02 10:08:40 PDT

All reviewed patches have been landed. Closing bug.

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