Last Comment Bug 758179 - Image script broken (images appear highlighted after clicking on thumbnail)
: Image script broken (images appear highlighted after clicking on thumbnail)
Status: RESOLVED FIXED
: regression, testcase
Product: Core
Classification: Components
Component: Selection (show other bugs)
: 12 Branch
: All All
: -- normal (vote)
: mozilla15
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
: Jet Villegas (:jet)
Mentors:
http://www.shellyburke.com/gallery.html
Depends on: 761483 787944
Blocks: 619273
  Show dependency treegraph
 
Reported: 2012-05-24 05:54 PDT by Loic
Modified: 2012-09-04 01:49 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
reduced html zip file (173.59 KB, application/java-archive)
2012-05-24 07:39 PDT, Alice0775 White
no flags Details
testcase #2 (460 bytes, text/html)
2012-05-27 21:44 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
Part 1: Flush layout before calling nsFrame::HandlePress/HandleRelease (1.19 KB, patch)
2012-05-28 04:48 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Part 2: Add flags parameter to GetContentOffsetsFromPoint(External) (4.34 KB, patch)
2012-05-28 04:49 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 3: Support SKIP_HIDDEN flag (13.12 KB, patch)
2012-05-28 04:49 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 4: Pass SKIP_HIDDEN flag in HandlePress/HandleRelease (2.77 KB, patch)
2012-05-28 04:50 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 5: Test (2.30 KB, patch)
2012-05-28 04:50 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
part 1 v2 (1.40 KB, patch)
2012-05-28 14:13 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
bugs: feedback+
Details | Diff | Splinter Review

Description Loic 2012-05-24 05:54:15 PDT
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
Comment 1 Alice0775 White 2012-05-24 06:30:10 PDT
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
Comment 2 Alice0775 White 2012-05-24 07:39:40 PDT
Created attachment 626802 [details]
reduced html zip file
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-24 16:14:54 PDT
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?
Comment 4 Mats Palmgren (:mats) 2012-05-25 06:33:13 PDT
I'd appreciate your help, thanks.
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-27 20:57:06 PDT
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.
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-27 21:00:43 PDT
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.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-27 21:44:58 PDT
Created attachment 627616 [details]
testcase #2

This testcase should show the problem on builds before bug 619273 was fixed.
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-27 23:03:26 PDT
(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.
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-28 04:48:54 PDT
Created attachment 627665 [details] [diff] [review]
Part 1: Flush layout before calling nsFrame::HandlePress/HandleRelease
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-28 04:49:26 PDT
Created attachment 627666 [details] [diff] [review]
Part 2: Add flags parameter to GetContentOffsetsFromPoint(External)
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-28 04:49:56 PDT
Created attachment 627667 [details] [diff] [review]
Part 3: Support SKIP_HIDDEN flag
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-28 04:50:25 PDT
Created attachment 627668 [details] [diff] [review]
Part 4: Pass SKIP_HIDDEN flag in HandlePress/HandleRelease
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-28 04:50:57 PDT
Created attachment 627669 [details] [diff] [review]
Part 5: Test
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-28 04:51:45 PDT
I've added a comment for SKIP_HIDDEN in patch 2:
// Treat visibility:hidden frames as non-selectable
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-28 05:19:24 PDT
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 16 Timothy Nikkel (:tnikkel) 2012-05-28 11:00:18 PDT
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?
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-28 14:08:11 PDT
I sure could!
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-28 14:13:25 PDT
Created attachment 627787 [details] [diff] [review]
part 1 v2
Comment 19 Mats Palmgren (:mats) 2012-05-28 14:29:27 PDT
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
Comment 20 Olli Pettay [:smaug] 2012-05-28 14:56:18 PDT
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.
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-28 16:33:08 PDT
I think we try to avoid flushing layout on mouse-move.
Comment 24 Loic 2012-06-01 10:29:39 PDT
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?
Comment 25 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-04 16:19:47 PDT
I think it's not quite worth uplifting this to beta (it's already on Aurora because the uplift happened today)

Note You need to log in before you can comment on or make changes to this bug.