Bug 57439

Summary: CSS property outline not displayed on an iframe
Product: WebKit Reporter: Miguel SilvaR <silvermik3>
Component: CSSAssignee: noel gordon <noel.gordon>
Status: RESOLVED FIXED    
Severity: Minor CC: commit-queue, dglazkov, eric, hyatt, inferno, jamesr, koz, lukasz.olejnik, mitz, morrita, noel.gordon, webkit.review.bot
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 58782    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch - followup
none
Patch
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-02
none
frameset-test.html
none
border styled frameset
none
outline styled frameset
none
frameset-test.html results for chrome11/safari5.0.4/firefox4
none
RenderWidget and related renderer classes
none
Patch
none
Archive of layout-test-results from cr-jail-8
none
Patch
none
Patch none

Description Miguel SilvaR 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.
Comment 1 Alexey Proskuryakov 2011-03-30 09:58:00 PDT
Outline for tables has been implemented this month in bug 55474.
Comment 2 Abhishek Arya 2011-04-13 09:42:29 PDT
Created attachment 89389 [details]
Patch
Comment 3 James Robinson 2011-04-13 12:32:53 PDT
Comment on attachment 89389 [details]
Patch

R=me
Comment 4 WebKit Commit Bot 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>
Comment 5 WebKit Commit Bot 2011-04-13 18:42:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 noel gordon 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 ...
Comment 7 Abhishek Arya 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.
Comment 8 noel gordon 2011-04-16 01:37:47 PDT
Thanks for the explanation Abhishek, I'll add a followup patch herein.
Comment 9 noel gordon 2011-04-16 01:42:16 PDT
Created attachment 89920 [details]
Patch - followup
Comment 10 mitz 2011-04-16 23:21:41 PDT
Looks like this change caused all plug-ins to have focus rings when focused.
Comment 11 mitz 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.
Comment 12 Abhishek Arya 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.
Comment 13 Abhishek Arya 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.
Comment 14 noel gordon 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.
Comment 15 noel gordon 2011-05-02 02:41:24 PDT
Created attachment 91900 [details]
Patch
Comment 16 noel gordon 2011-05-03 21:28:05 PDT
Created attachment 92189 [details]
Patch
Comment 17 WebKit Review Bot 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
Comment 18 WebKit Review Bot 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
Comment 19 Lukasz Olejnik 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
Comment 20 Ojan Vafai 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");
Comment 21 noel gordon 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
Comment 22 noel gordon 2011-05-27 00:29:12 PDT
Created attachment 95128 [details]
frameset-test.html
Comment 23 noel gordon 2011-05-27 00:29:43 PDT
Created attachment 95129 [details]
border styled frameset
Comment 24 noel gordon 2011-05-27 00:30:30 PDT
Created attachment 95130 [details]
outline styled frameset
Comment 25 noel gordon 2011-05-27 00:37:27 PDT
Created attachment 95131 [details]
frameset-test.html results for chrome11/safari5.0.4/firefox4
Comment 26 noel gordon 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.
Comment 27 Lukasz Olejnik 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?
Comment 28 noel gordon 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.
Comment 29 noel gordon 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.
Comment 30 noel gordon 2011-05-27 02:00:31 PDT
Created attachment 95147 [details]
RenderWidget and related renderer classes
Comment 31 noel gordon 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.
Comment 32 noel gordon 2011-05-27 03:02:37 PDT
Created attachment 95150 [details]
Patch
Comment 33 Eric Seidel (no email) 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?
Comment 34 noel gordon 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.
Comment 35 Eric Seidel (no email) 2011-05-29 14:38:51 PDT
Comment on attachment 95150 [details]
Patch

OK.  Seems reasonable.
Comment 36 noel gordon 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?
Comment 37 noel gordon 2011-06-01 19:20:25 PDT
CQ?
Comment 38 Eric Seidel (no email) 2011-06-01 21:18:17 PDT
Comment on attachment 95150 [details]
Patch

Haven't we made you a committer yet? :)
Comment 39 WebKit Commit Bot 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
Comment 40 WebKit Commit Bot 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
Comment 41 noel gordon 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.
Comment 42 noel gordon 2011-06-02 20:56:09 PDT
Created attachment 95850 [details]
Patch
Comment 43 noel gordon 2011-06-02 20:57:22 PDT
Text result was enough, so used that.
Comment 44 noel gordon 2011-06-06 01:08:32 PDT
> Haven't we made you a committer yet? :)

Nope :/
Comment 45 Hajime Morrita 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.
Comment 46 WebKit Review Bot 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>
Comment 47 WebKit Review Bot 2011-06-06 21:04:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 48 WebKit Commit Bot 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.
Comment 49 James Kozianski 2011-06-06 22:18:14 PDT
Rebaselines for windows/linux images committed in http://trac.webkit.org/changeset/88217.
Comment 50 Hajime Morrita 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>
Comment 51 Hajime Morrita 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.
Comment 52 noel gordon 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?
Comment 53 Hajime Morrita 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.
Comment 54 noel gordon 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.
Comment 55 noel gordon 2011-06-15 01:48:04 PDT
Created attachment 97256 [details]
Patch
Comment 56 Eric Seidel (no email) 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?
Comment 57 WebKit Review Bot 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>
Comment 58 WebKit Review Bot 2011-06-15 08:10:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 59 noel gordon 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?
Comment 60 Eric Seidel (no email) 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. :)
Comment 61 noel gordon 2011-06-19 22:50:51 PDT
IC, and thanks for the itemized list.  tells us where our focus should be ;)