The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla15

Status

()

Core
Selection
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Loic, Assigned: roc)

Tracking

({regression, testcase})

12 Branch
mozilla15
regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(7 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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
(Reporter)

Updated

5 years ago
Keywords: regression

Comment 1

5 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

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

5 years ago
Created attachment 626802 [details]
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.
Created attachment 627616 [details]
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.
Created attachment 627665 [details] [diff] [review]
Part 1: Flush layout before calling nsFrame::HandlePress/HandleRelease
Attachment #627665 - Flags: review?(matspal)
Created attachment 627666 [details] [diff] [review]
Part 2: Add flags parameter to GetContentOffsetsFromPoint(External)
Attachment #627666 - Flags: review?(matspal)
Created attachment 627667 [details] [diff] [review]
Part 3: Support SKIP_HIDDEN flag
Attachment #627667 - Flags: review?(matspal)
Created attachment 627668 [details] [diff] [review]
Part 4: Pass SKIP_HIDDEN flag in HandlePress/HandleRelease
Attachment #627668 - Flags: review?(matspal)
Created attachment 627669 [details] [diff] [review]
Part 5: Test
Attachment #627669 - Flags: review?(matspal)
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?
I sure could!
Created attachment 627787 [details] [diff] [review]
part 1 v2
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.

Updated

5 years ago
Attachment #627666 - Flags: review?(matspal) → review+

Updated

5 years ago
Attachment #627667 - Flags: review?(matspal) → review+

Updated

5 years ago
Attachment #627668 - Flags: review?(matspal) → review+

Updated

5 years ago
Attachment #627669 - Flags: review?(matspal) → review+
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
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
(Reporter)

Comment 24

5 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?
I think it's not quite worth uplifting this to beta (it's already on Aurora because the uplift happened today)
Depends on: 761483

Updated

5 years ago
Depends on: 787944
You need to log in before you can comment on or make changes to this bug.