Closed Bug 758179 Opened 12 years ago Closed 12 years ago

Image script broken (images appear highlighted after clicking on thumbnail)

Categories

(Core :: DOM: Selection, defect)

12 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: epinal99-bugzilla2, Assigned: roc)

References

()

Details

(Keywords: regression, testcase)

Attachments

(7 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120420145725

Steps to reproduce:

It's probably a regression in FF11+. Feel free to change the title to something more accurate and technical.

STR:
1) Open this image gallery: http://www.shellyburke.com/gallery.html
2) Click on a painting thumbnail in the image slider/carousel at the bottom




Actual results:

The full image appears highlighted if it had been selected with the mouse (blue overlay).
Click again on the thumbnail and the the overlay disappears (as it should be).

Regression range:

m-c:
good: 2011-12-20
bad: 2011-12-21
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2afd7ae68e8b&tochange=cd921d073b22

m-i:
good: 2011-12-20
bad: 2011-12-21
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=feaccb6a4c35&tochange=c78f7c5e1736
Regression window
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/feaccb6a4c35
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111219 Firefox/11.0a1 ID:20111219235256
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/0aa9c3a5b7e1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111219 Firefox/11.0a1 ID:20111220011653
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=feaccb6a4c35&tochange=0aa9c3a5b7e1

Triggered by : Bug 619273
Blocks: 619273
Component: Untriaged → Selection
Product: Firefox → Core
QA Contact: untriaged → selection
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached file reduced html zip file
Attachment #626802 - Attachment mime type: application/x-zip → application/java-archive
Testcase: jar:https://bug758179.bugzilla.mozilla.org/attachment.cgi?id=626802!/index.html
(Could easily be reduced to a single file using data: URIs though)

Mats, do you want this or should I fix it?
I'd appreciate your help, thanks.
Assignee: nobody → roc
Keywords: testcase
OS: Windows 7 → All
Hardware: x86_64 → All
What happens in this testcase is that the <img> is hidden, we click in the <div>, nsFrame::HandlePress calls GetContentOffsetsFromPoint without flushing layout, which scans through the child frames of the <div> without checking CSS 'visibility' and identifies the <img> as the selected frame under the point. So the image gets selected.

Before bug 619273, I presume the same thing would happen but setting the "chosen" class caused a style change to position:absolute, which reconstructed the frame for the <img> meaning we lost the selection state. Bug 619273 fixed that bug and exposed this bug.
Arguably, GetContentOffsetsFromPoint and/or HandlePress should work differently and ignore visibility:hidden frames.

However, a more obvious issue is that we should flush layout between the DOM event handling and HandlePress looking at frame geometry. That would fix this bug.
Attached file testcase #2
This testcase should show the problem on builds before bug 619273 was fixed.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> However, a more obvious issue is that we should flush layout between the DOM
> event handling and HandlePress looking at frame geometry. That would fix
> this bug.

Turns out that this doesn't fix this bug, obviously. If the event listener is on 'mouseup' or 'click', then the mouse-down will select the hidden <img> element, and then the mouseup/click handler will make it visible.
I've added a comment for SKIP_HIDDEN in patch 2:
// Treat visibility:hidden frames as non-selectable
GetContentOffsetsFromPoint is mysterious, inconsistent and probably broken. However, there isn't much cross-browser consistency in what gets selected when you click on content like this. I've made the test not check for particular nodes or offsets, just ensuring that nothing is selected, so we can change the behavior in the future.
Comment on attachment 627665 [details] [diff] [review]
Part 1: Flush layout before calling nsFrame::HandlePress/HandleRelease

Can you make the comment explain why mouse up/down are special and need a flush and other events do not?
Attached patch part 1 v2Splinter Review
Attachment #627665 - Attachment is obsolete: true
Attachment #627665 - Flags: review?(matspal)
Attachment #627787 - Flags: review?(matspal)
Comment on attachment 627787 [details] [diff] [review]
part 1 v2

I can't help wondering why other event handlers don't want up-to-date
layout.  Why shouldn't this flush be unconditional?

Anyway, it would be good if Olli could have a look too, for a sanity
check of having a flush here.

r=mats
Attachment #627787 - Flags: review?(matspal)
Attachment #627787 - Flags: review+
Attachment #627787 - Flags: feedback?(bugs)
Comment on attachment 627787 [details] [diff] [review]
part 1 v2

It is ok to flush here.

We should probably check other nsIFrame::HandleEvent cases and
check if some other event might need flushing.
But in a different bug.
Attachment #627787 - Flags: feedback?(bugs) → feedback+
I think we try to avoid flushing layout on mouse-move.
Attachment #627666 - Flags: review?(matspal) → review+
Attachment #627667 - Flags: review?(matspal) → review+
Attachment #627668 - Flags: review?(matspal) → review+
Attachment #627669 - Flags: review?(matspal) → review+
I tested in the latest Nightly, it seems to be fixed, great.
Is there a plan to backport the patch in Aurora and/or Beta?
I think it's not quite worth uplifting this to beta (it's already on Aurora because the uplift happened today)
Depends on: 787944
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: