Closed Bug 424236 Opened 13 years ago Closed 13 years ago

Focus rectangle broken on anchors around block elements

Categories

(Core :: Layout: Block and Inline, defect, P2)

x86
All
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: ckowaliski, Assigned: dbaron)

References

(Depends on 1 open bug, )

Details

(Keywords: testcase)

Attachments

(3 files, 4 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4) Gecko/2008030714 Firefox/3.0b4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4) Gecko/2008030714 Firefox/3.0b4

When an image is inside anchor tags, clicking it normally draws a rectangle around it to highlight it. However, if you give the image the CSS attribute "display:block", Firefox 3.0b4 draws a tiny horizontal line slightly above it at the top left. Firefox 2.0.0.12 doesn't do this.

Reproducible: Always

Steps to Reproduce:
1. Find an image that links to something.
2. Make sure the image has its CSS "display" property set to "block".
2. Click on it.
Actual Results:  
Firefox 3.0b4 draws a tiny horizontal line slightly above the image at the top left.

Expected Results:  
Firefox should draw a rectangle around the image. (Firefox 2.0.0.12 does this)

Happens on http://fredded.eu/etc/ff3block.html and on http://techreport.com (when you click the large feature image at the top left of the page).
Flags: blocking-firefox3?
Correction: Firefox 2.0.0.12 doesn't draw a rectangle at all in the same instance.
Huh, that looks like the focus outline is getting completely bogus frame coordinates.. 
Status: UNCONFIRMED → NEW
Component: General → Layout: Block and Inline
Ever confirmed: true
Flags: blocking-firefox3?
OS: Windows Vista → All
Product: Firefox → Core
Version: unspecified → Trunk
(Moving blocking-fx3? -> blocking-1.9?)
Flags: blocking1.9?
+'ing w/ P2.  We should fix this.  Cyril, can you attach a test case please?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
This was caused by three different bugs; the last and largest portion by a bug in this range:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1204104360&maxdate=1204106459
could be bug 400057 or bug 416168.
This bug started with only tiny 2px dots and suddenly became a line on 27 Feb 2008.
Attached file testcase from URL
Testcase from URL; attached here so it doesn't get lost if the original site goes away.
This is actually worse, and might be a P1 blocker.  Any block element inside an anchor causes the problem.
Summary: Bad link highlight behavior on images set as display:block → Focus rectangle broken on anchors around block elements
Keywords: regression, testcase
QA Contact: general → layout.block-and-inline
Could also be bug 417255.
QA Contact: layout.block-and-inline → general
QA Contact: general → layout.block-and-inline
So the regression here looks like we're drawing a line instead of a dot; we never did the right thing.  (What we're drawing the outline around is the little bit of the inline before the block-inside-inline split containing the block level image.)

Do you think this is a blocker just because of that regression, or because we should fix the bug that the image isn't outlined, or for some other reason?
And yeah, the change from a dot to a line probably would have been caused by bug 416168.  (But bug 417255 is actually a bit related to fixing the real bug and getting the outline around the image.)
Yeah, not a regression it turns out.. I could've sworn I tested this in an earlier build and had the focus outline drawn correctly, but I guess not.  Fx2 was drawing an outline around a dot which was very hard to see, so this has been broken for a while.  It's just more visibly broken now.  Still, we should probably fix for final.
Keywords: regression
Attached patch patch to outline the image (obsolete) — Splinter Review
Here's a patch to outline the image.  If you think this is worth doing at this point, I'll write some reftests.
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Attachment #311436 - Flags: superreview?(roc)
Attachment #311436 - Flags: review?(roc)
Blocks: 270191
Attachment #311436 - Flags: superreview?(roc)
Attachment #311436 - Flags: superreview+
Attachment #311436 - Flags: review?(roc)
Attachment #311436 - Flags: review+
I don't really mind whether we take the patch or not, but either way we should just close this bug out now.
Whiteboard: [reviewed patch in hand]
Comment on attachment 311436 [details] [diff] [review]
patch to outline the image

a=me, but needs reftest to land
Attachment #311436 - Flags: approval1.9+
Attached patch patch to outline the image (obsolete) — Splinter Review
This has reftests added, and fixes a bug they turned up -- my code to delve in to the blocks inside the wrappers didn't add the outline width + offset again (it's normally already added to the overflow area).  While doing this, I moved the declarations of three variables closer to their initialization -- important since otherwise I'd have risked using one of them uninitialized.
Attachment #311436 - Attachment is obsolete: true
Attachment #313210 - Flags: superreview?(roc)
Attachment #313210 - Flags: review?(roc)
Attachment #313210 - Flags: superreview?(roc)
Attachment #313210 - Flags: review?(roc)
Yet another thing I realized I should test -- and then got wrong.  I need to inherit -moz-outline-offset too.  And handle negative outline-offsets correctly when munging overflow rects.
Attachment #313210 - Attachment is obsolete: true
Attachment #313217 - Flags: superreview?(roc)
Attachment #313217 - Flags: review?(roc)
Attachment #313217 - Flags: superreview?(roc)
Attachment #313217 - Flags: superreview+
Attachment #313217 - Flags: review?(roc)
Attachment #313217 - Flags: review+
I spun off bug 426879 on the underlying cause of the actual regression (point to line).  I think the right fix there is probably too scary for 1.9 at this point, although it might be possible to do some sort of partial revert of bug 416168 (making an exception for inline frames).
This just fixes the regression that this bug was complaining about.  I'm really not sure how to reftest this, since it brings us back to the misplaced dot that we used to have.

Thoughts on whether to take this, the other patch, or both?

I'm really not sure how to reftest this one, given that the dot is misplaced and I'm not quite sure how to compute its position reliably.  I could probably test with a != test, at least.
Attachment #313463 - Flags: superreview?(roc)
Attachment #313463 - Flags: review?(roc)
With reftest.  Question in previous comment still applies.

Note that if I land this without attachment 313217 [details] [diff] [review], I'll need to adjust the reference rendering in the reftest.
Attachment #313463 - Attachment is obsolete: true
Attachment #313467 - Flags: superreview?(roc)
Attachment #313467 - Flags: review?(roc)
Attachment #313463 - Flags: superreview?(roc)
Attachment #313463 - Flags: review?(roc)
Comment on attachment 313467 [details] [diff] [review]
lower-risk patch to just fix the regression

I can't decide which patch to take either. I think either one would be OK.
Attachment #313467 - Flags: superreview?(roc)
Attachment #313467 - Flags: superreview+
Attachment #313467 - Flags: review?(roc)
Attachment #313467 - Flags: review+
I'm deciding between:
 * taking both attachment 313217 [details] [diff] [review] and attachment 313467 [details] [diff] [review]
 * taking only attachment 313467 [details] [diff] [review]

I think I'm inclined towards the latter.
If the latter, should mark this as wanted-next at least to pick up the real fix as soon as the tree opens for post-1.9.0 work..
(In reply to comment #22)
> If the latter, should mark this as wanted-next at least to pick up the real fix
> as soon as the tree opens for post-1.9.0 work..
> 

Let's just take the low risk one and close this out..
Low risk patch checked in to trunk, 2008-04-05 20:55 -07:00.  I have the other patch in my mq queue ready to land once mozilla-central opens.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
I still see a regression with Firefox 3.0 RC1. For an image with display:block inside an focused anchor, Firefox draws 2x2px rectangles on some of the image corners.
This is roughly the parts of the patch in bug 434301 that addresses
this bug.  I've included the changes I did to the pending reftests.
Hopefully the Bugzilla patch interdiff function will work...
I landed http://hg.mozilla.org/mozilla-central/index.cgi/rev/2018e7c14383
... so perhaps we should continue on bug 434301 now, but with a patch diffing against what's now landed?
Comment on attachment 322433 [details] [diff] [review]
patch to outline the image, rev. 2

Posted updated patch in bug 434301.
Attachment #322433 - Attachment is obsolete: true
Depends on: 444036
Flags: in-testsuite?
Flags: in-testsuite?
Whiteboard: [reviewed patch in hand]
You need to log in before you can comment on or make changes to this bug.