Bug 57439 – CSS property outline not displayed on an iframe

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Miguel SilvaR]

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.

[Alexey Proskuryakov]

Outline for tables has been implemented this month in bug 55474.

[Abhishek Arya]

Created attachment 89389 [details]
Patch

[James Robinson]

Comment on attachment 89389 [details]
Patch

R=me

[WebKit Commit Bot]

Comment on attachment 89389 [details]
Patch

Clearing flags on attachment: 89389

Committed r83803: <http://trac.webkit.org/changeset/83803>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

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]

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]

Thanks for the explanation Abhishek, I'll add a followup patch herein.

[noel gordon]

Created attachment 89920 [details]
Patch - followup

Looks like this change caused all plug-ins to have focus rings when focused.

(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 on attachment 89920 [details]
Patch - followup

I am rolling out the whole patch. So, marking this as obsolete for now.

[Abhishek Arya]

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]

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]

Created attachment 91900 [details]
Patch

[noel gordon]

Created attachment 92189 [details]
Patch

[WebKit Review Bot]

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]

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]

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

(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]

Created attachment 95128 [details]
frameset-test.html

[noel gordon]

Created attachment 95129 [details]
border styled frameset

[noel gordon]

Created attachment 95130 [details]
outline styled frameset

[noel gordon]

Created attachment 95131 [details]
frameset-test.html results for chrome11/safari5.0.4/firefox4

[noel gordon]

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]

(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]

(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]

(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]

Created attachment 95147 [details]
RenderWidget and related renderer classes

[noel gordon]

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]

Created attachment 95150 [details]
Patch

[Eric Seidel (no email)]

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]

(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 on attachment 95150 [details]
Patch

OK.  Seems reasonable.

[noel gordon]

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]

CQ?

[Eric Seidel (no email)]

Comment on attachment 95150 [details]
Patch

Haven't we made you a committer yet? :)

[WebKit Commit Bot]

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]

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]

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]

Created attachment 95850 [details]
Patch

[noel gordon]

Text result was enough, so used that.

[noel gordon]

> Haven't we made you a committer yet? :)

Nope :/

[Hajime Morrita]

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 on attachment 95850 [details]
Patch

Clearing flags on attachment: 95850

Committed r88212: <http://trac.webkit.org/changeset/88212>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

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]

Rebaselines for windows/linux images committed in http://trac.webkit.org/changeset/88217.

[Hajime Morrita]

Reverted r88212 for reason:

Tests get timeout at Snow Leopard

Committed r88228: <http://trac.webkit.org/changeset/88228>

[Hajime Morrita]

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]

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

(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]

(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]

Created attachment 97256 [details]
Patch

[Eric Seidel (no email)]

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 on attachment 97256 [details]
Patch

Clearing flags on attachment: 97256

Committed r88934: <http://trac.webkit.org/changeset/88934>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

> 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)]

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]

IC, and thanks for the itemized list.  tells us where our focus should be ;)