If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

'outline' (focus outline) on inline containing only block causes visual artifact (extra dots) {ib}

NEW
Assigned to

Status

()

Core
Layout: Block and Inline
10 years ago
a year ago

People

(Reporter: Kai de Leeuw, Assigned: mats)

Tracking

(Blocks: 1 bug, {testcase})

Trunk
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 5 obsolete attachments)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008051706 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008051706 Minefield/3.0pre

See attached minimized testcase.

Reproducible: Always

Steps to Reproduce:
1.Load testcase
2.Press tab until link is focused
3.Behold the artifact above the link to the left
Actual Results:  
Artifact visible

Expected Results:  
Artifact not visible
(Reporter)

Comment 1

10 years ago
Created attachment 321474 [details]
Testcase that demonstrates the problem describe in summary
(Reporter)

Updated

10 years ago
Keywords: testcase
(Reporter)

Comment 2

10 years ago
Created attachment 321476 [details]
Screen shot that demonstrates the problem describe in summary

Comment 3

10 years ago
Block-level elements are not allowed inside inline elements.  If you set display:block on the image, then you must do so for the anchor as well.
Whiteboard: [invalid?]
(Reporter)

Comment 4

10 years ago
yeah, you are right... I feel ashamed I didn't get this earlier...
Status: UNCONFIRMED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → INVALID

Comment 5

10 years ago
(In reply to comment #3)
> Block-level elements are not allowed inside inline elements.  If you set
> display:block on the image, then you must do so for the anchor as well.
> 

You're confusing HTML4 %block and %inline with CSS display:block and display:inline.

CSS does not forbid display:block inside display:inline.
(Reporter)

Comment 6

10 years ago
So how should it be displayed?

Now it looks like this:

|      <------- this is in some older builds that I checked, like a tail sticking
-------         out from on top of the pic with focus outline, in later builds, 
| pic |         the dot
-------
What CSS says is that inlines that contain blocks get split into a piece before the block and a piece after the block; you're seeing the piece before the block get an outline around it.  That behavior probably isn't ideal...

See also bug 424236, bug 270191.
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
(Assignee)

Comment 8

10 years ago
Created attachment 321689 [details]
Testcase #2

There seems to be some difference between UAs on this testcase.

Safari 3.1 and latest Webkit:
  no outline around empty inlines, except if 'background'
  is set (huh?).  (ie. 3 green,pruple boxes)

Opera 9.50b2 (Linux)
  no outline around empty inlines or blocks
  (and shows an outline around the focused block-in-inline block box)
Opera 9.27 (Linux)
  no outline around empty inlines
  (no outline around the focused block-in-inline block box)

IE8b1 (Windows XPSP3)
  outline around empty inlines and blocks, looks close
  to what we used to implemented before bug 424236.
(Assignee)

Comment 9

10 years ago
Created attachment 321691 [details] [diff] [review]
wip1

The change in 424236 also targets normal inlines, was that intentional?

Perhaps something like this would be better if we want to target the
block-in-inline problem specifically.
Note that one of the patches in bug 424236 isn't actually landed yet (I've been waiting for mozilla-central to open).

Right now the part of the inline after the block essentially disappears; it would be nice if the part before it did as well.  (Do we even construct frames for the part after when it would be empty?)

I think your approach is reasonable, though.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

10 years ago
Blocks: 426879
OS: Windows XP → All
Hardware: PC → All
Whiteboard: [invalid?]
Version: unspecified → Trunk
(Assignee)

Comment 11

10 years ago
Created attachment 321876 [details]
Testcase #2e
(Assignee)

Comment 12

10 years ago
Created attachment 321877 [details]
Testcase #2f
(Assignee)

Comment 13

10 years ago
Created attachment 321878 [details]
Testcase #2g
(Assignee)

Comment 14

10 years ago
(In reply to comment #10)
> Note that one of the patches in bug 424236 isn't actually landed yet

Yes -- it looks like it fixes the painting of the outline(s) for the
block-in-inline block part.  I was mostly concerned with the rendering of
normal empty inlines here, which I now see really is bug 426879.

> Do we even construct frames for the part after when it would be empty?

If there are no child frames for it, we don't.
But we do create it if there is a zero-width child frame.
(Assignee)

Comment 15

10 years ago
Created attachment 321880 [details] [diff] [review]
wip3

There are several issues:
1. we should not paint outline for "empty" special frames
2. the outlines we skip should not contribute to the overflow rect
3. we should include outlines we do paint in the proper overflow rect,
   i.e. the painting we intend to do in bug 424236 should contribute
   to each of those child frame's overflow rect -- not the anon block
   overflow rect. Otherwise the surrounding overflow will not be
   correct and damage rect calculations will also be incorrect.

and "empty" in 1. means the overflow rect without outline added
has zero width or height. (the difference can be seen in test #2g
where "after" should have an outline, even though GetRect() is zero)

You can see the problem with 3. in test #2f, using the pending patch
in bug 424236, the right part of the outline inside the last two
blocks are not refreshed properly (try scrolling/covering the window).
And the horizontal scrollbars should not be here (problem 2).

This patch fixes 1 and 2 (although a bit hacky since
IsEmptySpecialFrame() means two different things depending on when
the call is done)
Attachment #321691 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Assignee: nobody → mats.palmgren
(Assignee)

Comment 16

10 years ago
Created attachment 322411 [details] [diff] [review]
Patch rev. 5

I found the problems with the patch in bug 424236: the paint problem
is caused by an optimisation in nsDisplayOutline::OptimizeVisibility()
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsDisplayList.cpp&rev=3.44&root=/cvsroot&mark=586-587#573
and the scrollbar is caused by the overflow rect for the anon block
always includes the outline - we should only include it where the
outline around a child will overflow its frame rect.

AFAICT, this patch fixes this bug, bug 426879 and the remaining part
for bug 424236.  Sorry for bundling them together but I found that
some of the code could be shared.
Attachment #321880 - Attachment is obsolete: true
Attachment #322411 - Flags: superreview?(dbaron)
Attachment #322411 - Flags: review?(dbaron)
(Assignee)

Comment 17

10 years ago
Created attachment 322412 [details] [diff] [review]
reftest2.diff

Reftests based on the attached testcases in this bug.
It looks like this patch incorporates the unlanded patch on bug 424236 (but not its tests).  So what I really need to review is the interdiff between the two.

(For what it's worth, I find it easier to review smaller patches, and I don't think it's a problem if one patch requires that another be applied first.  It's been pretty easy for me to do this since I've been using http://www.selenic.com/mercurial/wiki/index.cgi , although mq has its quirks and it's possible to mess things up pretty badly (including potentially losing work) until you get the hang of it.  If you try it remember to (1) use a versioned patch repository (hg init -c, and then hg qcommit frequently) and (2) set the default diff format to git diffs in ~/.hgrc by adding git = 1 in a [diff] section.)

Anyway, have to run now, so can't actually do the review now...
(Assignee)

Comment 19

10 years ago
Created attachment 322434 [details] [diff] [review]
reftest3.diff

Added the pending reftests from bug 424236, with some adjustments
to account for the fix in this bug.
Attachment #322412 - Attachment is obsolete: true
Could you update the patch now that bug 424236 landed:
  http://hg.mozilla.org/mozilla-central/index.cgi/rev/2018e7c14383
?
(Assignee)

Comment 21

10 years ago
Created attachment 324130 [details] [diff] [review]
Patch rev. 6
Attachment #322411 - Attachment is obsolete: true
Attachment #324130 - Flags: superreview?(dbaron)
Attachment #324130 - Flags: review?(dbaron)
Attachment #322411 - Flags: superreview?(dbaron)
Attachment #322411 - Flags: review?(dbaron)
(Assignee)

Comment 22

10 years ago
Created attachment 324131 [details] [diff] [review]
reftest4.diff

Reftests pass locally on Windows/Linux/OSX.
Attachment #322434 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Duplicate of this bug: 357125

Updated

9 years ago
Duplicate of this bug: 441989
Comment on attachment 324130 [details] [diff] [review]
Patch rev. 6

>-  nsIFrame *frameForArea = aForFrame;
>-  do {
>-    nsIAtom *pseudoType = frameForArea->GetStyleContext()->GetPseudoType();
>-    if (pseudoType != nsCSSAnonBoxes::mozAnonymousBlock &&
>-        pseudoType != nsCSSAnonBoxes::mozAnonymousPositionedBlock)
>-      break;
>-    // If we're done, we really want it and all its later siblings.
>-    frameForArea = frameForArea->GetFirstChild(nsnull);
>-    NS_ASSERTION(frameForArea, "anonymous block with no children?");
>-  } while (frameForArea);

As far as I can tell, you don't have any code that replaces this loop.  I just added a reftest for this as 424236-11.html; presumably this patch breaks that test.
Comment on attachment 324130 [details] [diff] [review]
Patch rev. 6

review- primarily because of the previous comment.  However, I'd add:

>+nsLayoutUtils::ComputeChildOverflowAreaUnion(const nsIFrame* aFrame)
>+{
>+  nsRect overflowArea = nsRect(0, 0, 0, 0);

I'd prefer default initialization, since the concept we want here is "no rectangle" rather than the particular one you give; they just happen to be represented the same way right now.

(in class nsLayoutUtils)
>+  static PRBool IsSpecialAnonymousBlockFrame(const nsIFrame* aFrame)

I don't see any reason for this to be inline.

>diff --git a/layout/generic/nsFrame.cpp b/layout/generic/nsFrame.cpp
>+  // Skip special frames with an empty overflow rect (bug 434301).
>+  // See also ::ComputeOutlineRect().
>+  if ((GetStateBits() & NS_FRAME_IS_SPECIAL) && GetOverflowRect().IsEmpty())
>+    return NS_OK;

Shouldn't we do this only for the inlines?  I tend to think we actually don't want to skip the blocks.

>+static nsRect
>+ComputeOutlineRect(const nsIFrame* aFrame, PRBool* aAnyOutline,
>+                   const nsRect& aOverflowRect)

>+  if (nsLayoutUtils::IsSpecialAnonymousBlockFrame(aFrame)) {
>+    NS_ASSERTION(aFrame->GetFirstChild(nsnull), "anonymous block must have children");
>+    nsRect r = nsLayoutUtils::ComputeChildOverflowAreaUnion(aFrame);
>+    r.Inflate(inflateBy, inflateBy);
>+    r.UnionRect(aOverflowRect, r);
>+    return r;
>+  }

This UnionRect means we're actually returning something bigger than the outline rect.  It seems like ComputeOutlineRect should be renamed to something that describes what it really does.


Other than those issues (previous comment and this one), this looks good.
Attachment #324130 - Flags: superreview?(dbaron)
Attachment #324130 - Flags: superreview-
Attachment #324130 - Flags: review?(dbaron)
Attachment #324130 - Flags: review-
(In reply to comment #26)
> (From update of attachment 324130 [details] [diff] [review])
> review- primarily because of the previous comment.  However, I'd add:
> 
> >+nsLayoutUtils::ComputeChildOverflowAreaUnion(const nsIFrame* aFrame)
> >+{
> >+  nsRect overflowArea = nsRect(0, 0, 0, 0);
> 
> I'd prefer default initialization, since the concept we want here is "no
> rectangle" rather than the particular one you give; they just happen to be
> represented the same way right now.

Given the changes in bug 439567, feel free to ignore this comment if you want.

However, you probably want to update your unioning to IncludeEmpty, as there.

Any chance of a new patch here?
Duplicate of this bug: 465070
Duplicate of this bug: 477370
Summary: IMG with display:block, wrapped in an anchor and a paragraph causes artifact to be visible when the anchor is focused → 'outline' (focus outline) on inline containing only block causes visual artifact (extra dots)
Bug 477370 has a bunch of good testcases.
Note that fixing bug 470118 would likely also fix this if we fixed the asymmetry by creating the inline on neither side.
Depends on: 470118

Comment 32

9 years ago
(In reply to comment #31)
> Note that fixing bug 470118 would likely also fix this if we fixed the
> asymmetry by creating the inline on neither side.

The original testcase in bug 477370 had extra whitespace inside the inline but not inside the block.
Duplicate of this bug: 625946
Summary: 'outline' (focus outline) on inline containing only block causes visual artifact (extra dots) → 'outline' (focus outline) on inline containing only block causes visual artifact (extra dots) {ib}
Why do we need the empty inline boxes on either side of the block box?  Wouldn't it be simpler and more efficient to avoid creating those in the first place, rather than having to recognize and ignore them all over layout?

Updated

6 years ago
Blocks: 680807
Duplicate of this bug: 680807
Duplicate of this bug: 1205254
You need to log in before you can comment on or make changes to this bug.