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)
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: epinal99-bugzilla2, Assigned: roc)
References
()
Details
(Keywords: regression, testcase)
Attachments
(7 files, 1 obsolete file)
173.59 KB,
application/java-archive
|
Details | |
460 bytes,
text/html
|
Details | |
4.34 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
13.12 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
2.77 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
2.30 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
1.40 KB,
patch
|
MatsPalmgren_bugz
:
review+
smaug
:
feedback+
|
Details | Diff | Splinter Review |
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
Keywords: regression
Comment 1•12 years ago
|
||
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
Updated•12 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 2•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #626802 -
Attachment mime type: application/x-zip → application/java-archive
Assignee | ||
Comment 3•12 years ago
|
||
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•12 years ago
|
||
I'd appreciate your help, thanks.
Assignee | ||
Comment 5•12 years ago
|
||
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.
Assignee | ||
Comment 6•12 years ago
|
||
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.
Assignee | ||
Comment 7•12 years ago
|
||
This testcase should show the problem on builds before bug 619273 was fixed.
Assignee | ||
Comment 8•12 years ago
|
||
(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.
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #627665 -
Flags: review?(matspal)
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #627666 -
Flags: review?(matspal)
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #627667 -
Flags: review?(matspal)
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #627668 -
Flags: review?(matspal)
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #627669 -
Flags: review?(matspal)
Assignee | ||
Comment 14•12 years ago
|
||
I've added a comment for SKIP_HIDDEN in patch 2: // Treat visibility:hidden frames as non-selectable
Assignee | ||
Comment 15•12 years ago
|
||
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•12 years ago
|
||
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?
Assignee | ||
Comment 17•12 years ago
|
||
I sure could!
Assignee | ||
Comment 18•12 years ago
|
||
Attachment #627665 -
Attachment is obsolete: true
Attachment #627665 -
Flags: review?(matspal)
Attachment #627787 -
Flags: review?(matspal)
Comment 19•12 years ago
|
||
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 20•12 years ago
|
||
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+
Assignee | ||
Comment 21•12 years ago
|
||
I think we try to avoid flushing layout on mouse-move.
Updated•12 years ago
|
Attachment #627666 -
Flags: review?(matspal) → review+
Updated•12 years ago
|
Attachment #627667 -
Flags: review?(matspal) → review+
Updated•12 years ago
|
Attachment #627668 -
Flags: review?(matspal) → review+
Updated•12 years ago
|
Attachment #627669 -
Flags: review?(matspal) → review+
Assignee | ||
Comment 22•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e10eadfc8734 https://hg.mozilla.org/integration/mozilla-inbound/rev/d8688d0836cf https://hg.mozilla.org/integration/mozilla-inbound/rev/d0a12b20ca95 https://hg.mozilla.org/integration/mozilla-inbound/rev/80b83c01e30b https://hg.mozilla.org/integration/mozilla-inbound/rev/d0beb7e43cc4
Comment 23•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e10eadfc8734 https://hg.mozilla.org/mozilla-central/rev/d8688d0836cf https://hg.mozilla.org/mozilla-central/rev/d0a12b20ca95 https://hg.mozilla.org/mozilla-central/rev/80b83c01e30b https://hg.mozilla.org/mozilla-central/rev/d0beb7e43cc4
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Reporter | ||
Comment 24•12 years ago
|
||
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?
Assignee | ||
Comment 25•12 years ago
|
||
I think it's not quite worth uplifting this to beta (it's already on Aurora because the uplift happened today)
You need to log in
before you can comment on or make changes to this bug.
Description
•