WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch - followup
(1.50 KB, patch)
2011-04-16 01:42 PDT
,
noel gordon
no flags
Details
Formatted Diff
Diff
Patch
(33.37 KB, patch)
2011-05-02 02:41 PDT
,
noel gordon
no flags
Details
Formatted Diff
Diff
Patch
(33.11 KB, patch)
2011-05-03 21:28 PDT
,
noel gordon
no flags
Details
Formatted Diff
Diff
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
Details
frameset-test.html
(423 bytes, text/html)
2011-05-27 00:29 PDT
,
noel gordon
no flags
Details
border styled frameset
(389 bytes, text/html)
2011-05-27 00:29 PDT
,
noel gordon
no flags
Details
outline styled frameset
(393 bytes, text/html)
2011-05-27 00:30 PDT
,
noel gordon
no flags
Details
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
Details
RenderWidget and related renderer classes
(62.04 KB, image/png)
2011-05-27 02:00 PDT
,
noel gordon
no flags
Details
Patch
(33.32 KB, patch)
2011-05-27 03:02 PDT
,
noel gordon
no flags
Details
Formatted Diff
Diff
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
Details
Patch
(33.97 KB, patch)
2011-06-02 20:56 PDT
,
noel gordon
no flags
Details
Formatted Diff
Diff
Patch
(30.69 KB, patch)
2011-06-15 01:48 PDT
,
noel gordon
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 89389
[details]
Patch
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
Created
attachment 91900
[details]
Patch
noel gordon
Comment 16
2011-05-03 21:28:05 PDT
Created
attachment 92189
[details]
Patch
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
Created
attachment 95150
[details]
Patch
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
Created
attachment 95850
[details]
Patch
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
Created
attachment 97256
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug