RESOLVED FIXED 57439
CSS property outline not displayed on an iframe
https://bugs.webkit.org/show_bug.cgi?id=57439
Summary CSS property outline not displayed on an iframe
Miguel SilvaR
Reported 2011-03-30 03:41:10 PDT
1. Open this url: http://www.8sec.eu/outline-test/ 2. Click on the 'Click here to set an css outline' link 3. See how outline is applied on the table element but not on the iframe. The iframe should get similar outline as the table. Both Safari and Chrome have this issue. This has been tested in Firefox as well, and works fine with it.
Attachments
Patch (20.39 KB, patch)
2011-04-13 09:42 PDT, Abhishek Arya
no flags
Patch - followup (1.50 KB, patch)
2011-04-16 01:42 PDT, noel gordon
no flags
Patch (33.37 KB, patch)
2011-05-02 02:41 PDT, noel gordon
no flags
Patch (33.11 KB, patch)
2011-05-03 21:28 PDT, noel gordon
no flags
Archive of layout-test-results from ec2-cr-linux-02 (4.97 MB, application/zip)
2011-05-04 20:23 PDT, WebKit Review Bot
no flags
frameset-test.html (423 bytes, text/html)
2011-05-27 00:29 PDT, noel gordon
no flags
border styled frameset (389 bytes, text/html)
2011-05-27 00:29 PDT, noel gordon
no flags
outline styled frameset (393 bytes, text/html)
2011-05-27 00:30 PDT, noel gordon
no flags
frameset-test.html results for chrome11/safari5.0.4/firefox4 (36.47 KB, image/png)
2011-05-27 00:37 PDT, noel gordon
no flags
RenderWidget and related renderer classes (62.04 KB, image/png)
2011-05-27 02:00 PDT, noel gordon
no flags
Patch (33.32 KB, patch)
2011-05-27 03:02 PDT, noel gordon
no flags
Archive of layout-test-results from cr-jail-8 (191.59 KB, application/zip)
2011-06-02 01:32 PDT, WebKit Commit Bot
no flags
Patch (33.97 KB, patch)
2011-06-02 20:56 PDT, noel gordon
no flags
Patch (30.69 KB, patch)
2011-06-15 01:48 PDT, noel gordon
no flags
Alexey Proskuryakov
Comment 1 2011-03-30 09:58:00 PDT
Outline for tables has been implemented this month in bug 55474.
Abhishek Arya
Comment 2 2011-04-13 09:42:29 PDT
James Robinson
Comment 3 2011-04-13 12:32:53 PDT
Comment on attachment 89389 [details] Patch R=me
WebKit Commit Bot
Comment 4 2011-04-13 18:41:52 PDT
Comment on attachment 89389 [details] Patch Clearing flags on attachment: 89389 Committed r83803: <http://trac.webkit.org/changeset/83803>
WebKit Commit Bot
Comment 5 2011-04-13 18:42:01 PDT
All reviewed patches have been landed. Closing bug.
noel gordon
Comment 6 2011-04-15 17:45:41 PDT
Had prepared a patch for this also, so just curious - are the style visibility checks actually needed here? RenderReplaced::shouldPaint() checks for that or am I missing something ...
Abhishek Arya
Comment 7 2011-04-15 22:27:58 PDT
Yes the visibility check was redundant. At other places in RenderBlock, RenderTable, it was not. The same function does have this redundancy elsewhere too - if (!m_frameView || paintInfo.phase != PaintPhaseForeground || style()->visibility() != VISIBLE) return; I can remove it in the future, or if you want, you can have a followup patch to clean it up.
noel gordon
Comment 8 2011-04-16 01:37:47 PDT
Thanks for the explanation Abhishek, I'll add a followup patch herein.
noel gordon
Comment 9 2011-04-16 01:42:16 PDT
Created attachment 89920 [details] Patch - followup
mitz
Comment 10 2011-04-16 23:21:41 PDT
Looks like this change caused all plug-ins to have focus rings when focused.
mitz
Comment 11 2011-04-16 23:25:23 PDT
(In reply to comment #10) > Looks like this change caused all plug-ins to have focus rings when focused. Filed bug 58740.
Abhishek Arya
Comment 12 2011-04-18 07:08:20 PDT
Comment on attachment 89920 [details] Patch - followup I am rolling out the whole patch. So, marking this as obsolete for now.
Abhishek Arya
Comment 13 2011-04-18 07:46:55 PDT
original patch rolled out in http://trac.webkit.org/changeset/84143. i might not have time to look into this, i am already buried deep in fixing security bugs.
noel gordon
Comment 14 2011-05-02 02:38:50 PDT
Focus uses outline style too. So <embed> <object> <iframe> <applet> (render widgets) would gain a focus ring via outline drawing unless we prevent it.
noel gordon
Comment 15 2011-05-02 02:41:24 PDT
noel gordon
Comment 16 2011-05-03 21:28:05 PDT
WebKit Review Bot
Comment 17 2011-05-04 20:23:16 PDT
Attachment 92189 [details] did not pass chromium-ews: Output: http://queues.webkit.org/results/8553720 New failing tests: fast/replaced/outline-replaced-elements.html
WebKit Review Bot
Comment 18 2011-05-04 20:23:25 PDT
Created attachment 92366 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: Chromium Platform: Linux-2.6.35-22-virtual-x86_64-with-Ubuntu-10.10-maverick
Lukasz Olejnik
Comment 19 2011-05-09 04:58:44 PDT
Hi, The initial bug was submitted by a friend of mine. I have updated the online test to include frame (part of frameset) as well: http://www.8sec.eu/outline-test/ Adding border/outline to frame varies from browser to browser but ideally it should work in Chrome in the same way as adding border/outline to table or iframe. Do you want me to submit a separate bug for showing outline for frames or this can be done with the fix here? Thanks
Ojan Vafai
Comment 20 2011-05-20 11:14:42 PDT
Comment on attachment 92189 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92189&action=review I'd prefer someone more familiar with RenderWidget reviewed that bit of the patch. The rest looks good to me. Would be better to combine all the no-focus-ring-* tests into one since the differences between them are pretty minor. > LayoutTests/fast/replaced/no-focus-ring-applet.html:16 > + var c = getComputedStyle(element, null).getPropertyValue("outline-color"); > + var s = getComputedStyle(element, null).getPropertyValue("outline-style"); > + var w = getComputedStyle(element, null).getPropertyValue("outline-width"); I know this is just a test, but s/c/color, s/s/style and s/w/width would be nice. > LayoutTests/fast/replaced/no-focus-ring-applet.html:18 > + var noFocusRing = ("0pxnone" == w + s); This is weird. Would be more natural to do the following I think: var noFocusRing = (w == "0px" && s == "none");
noel gordon
Comment 21 2011-05-27 00:26:35 PDT
(In reply to comment #19) > Adding border/outline to frame varies from browser to browser but ideally it should work in Chrome in the same way as adding border/outline to table or iframe. See http://www.w3.org/TR/html5-diff - frameset and frame elements are excluded from HTML5, not sure if it's worth doing more work on them. > Do you want me to submit a separate bug for showing outline for frames or this can be done with the fix here? For testing, based on your test (thx), I placed a <frameset> with two <frame> elements inside an <iframe>. The <iframe> has 15px padding to move the <frameset> edge away from the <iframe> edge. I tested <frame>'s with border style, and, in a separate <iframe>, with outline style. The test is attached: frameset-test.html
noel gordon
Comment 22 2011-05-27 00:29:12 PDT
Created attachment 95128 [details] frameset-test.html
noel gordon
Comment 23 2011-05-27 00:29:43 PDT
Created attachment 95129 [details] border styled frameset
noel gordon
Comment 24 2011-05-27 00:30:30 PDT
Created attachment 95130 [details] outline styled frameset
noel gordon
Comment 25 2011-05-27 00:37:27 PDT
Created attachment 95131 [details] frameset-test.html results for chrome11/safari5.0.4/firefox4
noel gordon
Comment 26 2011-05-27 01:41:13 PDT
Firefox does not draw <frame> borders at all, and the outline result is possibly a bug (looks odd to me). Webkit draws the border interior to the <frame> edge. Outlines are drawn around borders in general, but here there's no space for an outline - the space is clipped by the <frameset> edge and the horizontal <frame> spacer.
Lukasz Olejnik
Comment 27 2011-05-27 01:46:01 PDT
(In reply to comment #26) > [...] > Webkit draws the border interior to the <frame> edge. Outlines are drawn around borders in general, but here > there's no space for an outline - the space is clipped by the <frameset> edge and the horizontal <frame> spacer. Which makes me think.... how would outlines look in a 100% width/height iframe? There won't be space for it either, right?
noel gordon
Comment 28 2011-05-27 01:56:30 PDT
(In reply to comment #20) > (From update of attachment 92189 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=92189&action=review > > I'd prefer someone more familiar with RenderWidget reviewed that bit of the patch. The rest looks good to me. I'll attach a picture of the RenderWidget world - reminds me I should add <canvas> and <media> elements to the outline-replaced-elements.html test.
noel gordon
Comment 29 2011-05-27 01:57:22 PDT
(In reply to comment #27) > Which makes me think.... how would outlines look in a 100% width/height iframe? There won't be space for it either, right? correct, no space.
noel gordon
Comment 30 2011-05-27 02:00:31 PDT
Created attachment 95147 [details] RenderWidget and related renderer classes
noel gordon
Comment 31 2011-05-27 02:59:20 PDT
n reply to comment #20) > Would be better to combine all the no-focus-ring-* tests into one since the differences between them are pretty minor. I'd prefer to keep them separate. The plugin testing infrastructure is sometimes flakey. I'd like the option for ports to skip individual tests as needed. I factored out the common code though. Added tests for the missing plugin case, simulate flash in DRT using the test plugin, and added <canvas> and <media> to the outline render tests, make it a pure pixel test. Add test expectation, tames a Linux EWS. > > LayoutTests/fast/replaced/no-focus-ring-applet.html:16 > > + var c = getComputedStyle(element, null).getPropertyValue("outline-color"); > > + var s = getComputedStyle(element, null).getPropertyValue("outline-style"); > > + var w = getComputedStyle(element, null).getPropertyValue("outline-width"); > > I know this is just a test, but s/c/color, s/s/style and s/w/width would be nice. Done. > > LayoutTests/fast/replaced/no-focus-ring-applet.html:18 > > + var noFocusRing = ("0pxnone" == w + s); > > This is weird. Would be more natural to do the following I think: > var noFocusRing = (w == "0px" && s == "none"); Done.
noel gordon
Comment 32 2011-05-27 03:02:37 PDT
Eric Seidel (no email)
Comment 33 2011-05-27 07:08:55 PDT
Comment on attachment 95150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95150&action=review > Source/WebCore/css/html.css:859 > +applet:focus, embed:focus, iframe:focus, object:focus { Seems this could just be combined with the previous rule. > Source/WebCore/rendering/RenderWidget.cpp:269 > + if (!m_frameView || paintInfo.phase != PaintPhaseForeground) Did you mean to remove the visibility check?
noel gordon
Comment 34 2011-05-27 07:41:00 PDT
(In reply to comment #33) > > > Source/WebCore/css/html.css:859 > > +applet:focus, embed:focus, iframe:focus, object:focus { > > Seems this could just be combined with the previous rule. Could be combined yes. I decided to let the comment @854 stand (you can't see it in review context). I'm easy either way so let me know. > > Source/WebCore/rendering/RenderWidget.cpp:269 > > + if (!m_frameView || paintInfo.phase != PaintPhaseForeground) > > Did you mean to remove the visibility check? I did. See comments #6 and #7 above.
Eric Seidel (no email)
Comment 35 2011-05-29 14:38:51 PDT
Comment on attachment 95150 [details] Patch OK. Seems reasonable.
noel gordon
Comment 36 2011-05-30 00:12:01 PDT
My tests use the webkit test plugin which causes issues on the webkit snow-leopard bots if I'm reading correctly - bug 36462, bug 32229 - anyhow cq?
noel gordon
Comment 37 2011-06-01 19:20:25 PDT
CQ?
Eric Seidel (no email)
Comment 38 2011-06-01 21:18:17 PDT
Comment on attachment 95150 [details] Patch Haven't we made you a committer yet? :)
WebKit Commit Bot
Comment 39 2011-06-02 01:32:10 PDT
Comment on attachment 95150 [details] Patch Rejecting attachment 95150 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2 Last 500 characters of output: tmlmp . http/tests/xmlhttprequest ................................................................................................................................................................................ http/tests/xmlhttprequest/web-apps ............... http/tests/xmlhttprequest/workers ........... http/tests/xmlviewer . http/tests/xmlviewer/dumpAsText ........... 823.31s total testing time 23726 test cases (99%) succeeded 1 test case (<1%) was new 16 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/8756670
WebKit Commit Bot
Comment 40 2011-06-02 01:32:14 PDT
Created attachment 95739 [details] Archive of layout-test-results from cr-jail-8 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: cr-jail-8 Port: Mac Platform: Mac OS X 10.6.7
noel gordon
Comment 41 2011-06-02 04:29:21 PDT
fast/replaced/outline-replaced-elements.html -> new .............. Seems I need render tree dump for this test. I'll prepare a new patch and upload it tomorrow.
noel gordon
Comment 42 2011-06-02 20:56:09 PDT
noel gordon
Comment 43 2011-06-02 20:57:22 PDT
Text result was enough, so used that.
noel gordon
Comment 44 2011-06-06 01:08:32 PDT
> Haven't we made you a committer yet? :) Nope :/
Hajime Morrita
Comment 45 2011-06-06 19:30:16 PDT
Comment on attachment 95850 [details] Patch r+ on behalf of Eric. Let's try cq. I'll land this if it fail.
WebKit Review Bot
Comment 46 2011-06-06 21:04:37 PDT
Comment on attachment 95850 [details] Patch Clearing flags on attachment: 95850 Committed r88212: <http://trac.webkit.org/changeset/88212>
WebKit Review Bot
Comment 47 2011-06-06 21:04:54 PDT
All reviewed patches have been landed. Closing bug.
WebKit Commit Bot
Comment 48 2011-06-06 21:51:29 PDT
The commit-queue encountered the following flaky tests while processing attachment 95850 [details]: http/tests/xmlhttprequest/cross-origin-preflight-get.html bug 53976 The commit-queue is continuing to process your patch.
James Kozianski
Comment 49 2011-06-06 22:18:14 PDT
Rebaselines for windows/linux images committed in http://trac.webkit.org/changeset/88217.
Hajime Morrita
Comment 50 2011-06-07 01:42:12 PDT
Reverted r88212 for reason: Tests get timeout at Snow Leopard Committed r88228: <http://trac.webkit.org/changeset/88228>
Hajime Morrita
Comment 51 2011-06-07 01:46:24 PDT
Rollout because following tests are timed-out on the build bot. - fast/replaced/no-focus-ring-embed-2.html -> timed out - fast/replaced/no-focus-ring-object-2.html -> timed out Both release and debug bots failed, so I thinks it's not just a flakiness.
noel gordon
Comment 52 2011-06-07 05:57:10 PDT
> Tests get timeout at Snow Leopard Snow Leopard DRT works for me locally of course. Morita-san, did the tests timeout on the Leopard bots?
Hajime Morrita
Comment 53 2011-06-07 22:38:32 PDT
(In reply to comment #52) > > Tests get timeout at Snow Leopard > > Snow Leopard DRT works for me locally of course. Morita-san, did the tests > timeout on the Leopard bots? It failed only on Snow Leopard release and debug bots. I have no idea why it failed... One idea is just to skip it for SL. It's kind of sad though.
noel gordon
Comment 54 2011-06-15 01:45:22 PDT
(In reply to comment #53) > It failed only on Snow Leopard release and debug bots. > I have no idea why it failed... > One idea is just to skip it for SL. It's kind of sad though. Worked on Leopard, worked on Chromium Snow Leopard, but not WebKit Snow Leopard. Appears you cannot focus the missing plugin on WebKit Snow Leopard. Confirmed that with more local testing. Tried loading a plugin with mine type "application/foo-bar" :) and observed the tests time-out. I'm removing those two tests therefore.
noel gordon
Comment 55 2011-06-15 01:48:04 PDT
Eric Seidel (no email)
Comment 56 2011-06-15 07:31:29 PDT
Comment on attachment 97256 [details] Patch LGTM. I take it we need to pass the Outline phase down to any children of this widget? that's why we don't just return right after painting?
WebKit Review Bot
Comment 57 2011-06-15 08:09:43 PDT
Comment on attachment 97256 [details] Patch Clearing flags on attachment: 97256 Committed r88934: <http://trac.webkit.org/changeset/88934>
WebKit Review Bot
Comment 58 2011-06-15 08:10:01 PDT
All reviewed patches have been landed. Closing bug.
noel gordon
Comment 59 2011-06-17 02:42:35 PDT
> LGTM. I take it we need to pass the Outline phase down to any children of this widget? that's why we don't just return right after painting? Nope. RenderWidgets along with <canvas>, <img> and friends, are RenderReplaced elements. So their children are meaningless - they are replaced (neglecting fallback content cases and oddities like a Flash Satay) and do not exist for styling per the CSS2.1 definition of replaced elements. The return early (or not) business is because I sensed an ambivalence in the code about the early return or not. The paintBorderDecorations bit could early return, but does not. Instead, it exits half way down the routine. Curious. I put my code just before the exit point as a compromise early out, but noted the amount of paint code after the exit point. There is a fair bit. So maybe the exit point is designed to prime the CPU instruction cache with what comes after (we know we're gonna need it soon)? Possible, and enough to convince me to not rewrite this "switch statement masquerading as a bunch of ifs" and alter paint performance. That could be the subject of another bug. Conversions to IntRect and tx/ty to Point are not free, but those changes lacked any paint performance measurements best I can tell. Should I care about performance?
Eric Seidel (no email)
Comment 60 2011-06-17 09:32:21 PDT
Paint performance is entirely dominated by: 1. mallocs 2. actually putting the bits on the screen (mallocs) 3. N^2 walks of the rendering tree (or other bad) algorithms. float to int conversions, early returns, etc. have never shown up on any profile i've seen in 6 years of doing this. :)
noel gordon
Comment 61 2011-06-19 22:50:51 PDT
IC, and thanks for the itemized list. tells us where our focus should be ;)
Note You need to log in before you can comment on or make changes to this bug.