Closed Bug 979692 Opened 11 years ago Closed 11 years ago

Add "hit" support to hit regions

Categories

(Core :: Graphics: Canvas2D, defect)

Product:
Core
Component:
Graphics: Canvas2D
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla32

People

(Reporter: cabanier, Assigned: cabanier)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 32 obsolete files)

Step 1: extend MouseEvent interface per the canvas spec
2.67 KB, patch
Details | Diff | Splinter Review
Step 2: Fix hitregion support to keep order of regions.
10.92 KB, patch
Details | Diff | Splinter Review
Step 4: added test for hit regions
3.32 KB, patch
Details | Diff | Splinter Review
Step 3: detect hit on region + fill in region attribute
11.66 KB, patch
roc
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → cabanier
Depends on: 966591
Attached patch 1: extend MouseEvent interface (obsolete) — — Splinter Review
Attached patch 1: extend MouseEvent interface (obsolete) — — Splinter Review
Attachment #8385816 - Attachment is obsolete: true
Attachment #8385853 - Attachment is obsolete: true
Attachment #8411378 - Attachment is obsolete: true
Attachment #8411383 - Flags: review?(roc)
Attachment #8411384 - Flags: review?(roc)
Comment on attachment 8411370 [details] [diff] [review] Step 1: extend MouseEvent interface per the canvas spec Try bot detected issue with this patch. Will fix that first.
Attachment #8411370 - Flags: review?(bzbarsky)
Comment on attachment 8411383 [details] [diff] [review] Step 2: Fix hitregion support to keep order of regions. Review of attachment 8411383 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/CanvasRenderingContext2D.h @@ +760,2 @@ > nsRefPtr<Element> mElement; > + RefPtr<gfx::Path> mPath; Document what coordinate system this path is in. @@ +762,3 @@ > }; > > + nsDeque mHitRegionsOptions; Why an nsDeque?
Attachment #8411383 - Flags: review?(roc) → review-
Comment on attachment 8411384 [details] [diff] [review] step 3: detect hit on region + fill in region attribute Review of attachment 8411384 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/CanvasRenderingContext2D.cpp @@ +1095,5 @@ > } > > +nsString CanvasRenderingContext2D::GetHitRegion(const mozilla::gfx::Point& aPoint) > +{ > + for(int32_t x = 0 ; x < mHitRegionsOptions.GetSize(); x++) { space before ( ::: content/html/content/src/HTMLCanvasElement.cpp @@ +298,5 @@ > + nsRefPtr<CanvasRenderingContext2D> context2d = > + static_cast<CanvasRenderingContext2D*>(cxt.get()); > + if (context2d) { > + CSSIntPoint p = Event::GetClientCoords(aVisitor.mPresContext, evt, evt->refPoint, CSSIntPoint(0, 0)); > + nsIFrame* boundingFrame = nsLayoutUtils::GetContainingBlockForClientRect(GetPrimaryFrame()); Use nsLayoutUtils::GetEventCoordinatesRelativeTo
Attachment #8411384 - Flags: review?(roc) → review-
Comment on attachment 8411383 [details] [diff] [review] Step 2: Fix hitregion support to keep order of regions. Review of attachment 8411383 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/CanvasRenderingContext2D.h @@ +760,2 @@ > nsRefPtr<Element> mElement; > + RefPtr<gfx::Path> mPath; will do. @@ +762,3 @@ > }; > > + nsDeque mHitRegionsOptions; Is there a better container that I could use?
Attachment #8411370 - Attachment is obsolete: true
Attachment #8412142 - Flags: review?(bzbarsky)
Attachment #8411383 - Attachment is obsolete: true
Attachment #8412185 - Flags: review?(roc)
Attachment #8411384 - Attachment is obsolete: true
Attachment #8412188 - Flags: review?(roc)
I don't really know how I can test this code. I can see that there's a way to synthesize an event and send it, but that doesn't really trigger the hit detection code.
synthesizeMouseEvent should work.
Comment on attachment 8412185 [details] [diff] [review] Step 2: Fix hitregion support to keep order of regions. Review of attachment 8412185 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/CanvasRenderingContext2D.cpp @@ +2460,5 @@ > + info->mElement = options.mControl; > + RefPtr<PathBuilder> pathBuilder = mPath->TransformedCopyToBuilder(mTarget->GetTransform()); > + info->mPath = pathBuilder->Finish(); > + > + mHitRegionsOptions.InsertElementsAt(0, 1, info); Just call InsertElementAt ::: content/canvas/src/CanvasRenderingContext2D.h @@ +763,3 @@ > }; > > + nsTArray<RegionInfo*> mHitRegionsOptions; Why not just nsTArray<RegionInfo> mHitRegionsOptions? Saves allocations.
Attachment #8412185 - Flags: review?(roc) → review-
Comment on attachment 8412188 [details] [diff] [review] step 3: detect hit on region + fill in region attribute Review of attachment 8412188 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/HTMLCanvasElement.cpp @@ +297,5 @@ > + GetContext(NS_LITERAL_STRING("2d"), getter_AddRefs(cxt)); > + nsRefPtr<CanvasRenderingContext2D> context2d = > + static_cast<CanvasRenderingContext2D*>(cxt.get()); > + if (context2d) { > + nsPoint ptInRoot = nsLayoutUtils::GetEventCoordinatesRelativeTo(evt, GetPrimaryFrame()); Hmm, can GetPrimaryFrame be null? Maybe so if a synthetic event is generated somehow. Better null check here. @@ +298,5 @@ > + nsRefPtr<CanvasRenderingContext2D> context2d = > + static_cast<CanvasRenderingContext2D*>(cxt.get()); > + if (context2d) { > + nsPoint ptInRoot = nsLayoutUtils::GetEventCoordinatesRelativeTo(evt, GetPrimaryFrame()); > + evt->region = context2d->GetHitRegion(Point(ptInRoot.x / AppUnitsPerCSSPixel(), ptInRoot.y / AppUnitsPerCSSPixel())); You need to subtract off the offset given by GetContentRectRelativeToSelf, to account for borders and padding on the canvas element.
Attachment #8412188 - Flags: review?(roc) → review-
Comment on attachment 8412185 [details] [diff] [review] Step 2: Fix hitregion support to keep order of regions. Review of attachment 8412185 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/CanvasRenderingContext2D.cpp @@ +2460,5 @@ > + info->mElement = options.mControl; > + RefPtr<PathBuilder> pathBuilder = mPath->TransformedCopyToBuilder(mTarget->GetTransform()); > + info->mPath = pathBuilder->Finish(); > + > + mHitRegionsOptions.InsertElementsAt(0, 1, info); done ::: content/canvas/src/CanvasRenderingContext2D.h @@ +763,3 @@ > }; > > + nsTArray<RegionInfo*> mHitRegionsOptions; From the comments in nsTArray.h: // T MUST be safely memmove()'able. I tried to make it store objects but ran into crashes when inserting. It seems that nsTArray_CopyChooser has bugs
Attachment #8412185 - Attachment is obsolete: true
Attachment #8412781 - Flags: review?(roc)
Attachment #8412188 - Attachment is obsolete: true
Attachment #8412782 - Flags: review?(roc)
Attachment #8412782 - Attachment is obsolete: true
Attachment #8412782 - Flags: review?(roc)
Attachment #8412969 - Flags: review?(roc)
Attachment #8412972 - Flags: review?(roc)
Attachment #8412972 - Attachment is obsolete: true
Attachment #8412972 - Flags: review?(roc)
Attachment #8412981 - Flags: review?(roc)
Attachment #8412969 - Attachment is obsolete: true
Attachment #8412969 - Flags: review?(roc)
Attachment #8413068 - Flags: review?(roc)
Comment on attachment 8412142 [details] [diff] [review] Step 1: extend MouseEvent interface per the canvas spec SetDOMStringToNull(aRegion) is what's usually done. r=me with that. Olli, do we really need to add this to nsIDOMMouseEvent?
Attachment #8412142 - Flags: review?(bzbarsky)
Attachment #8412142 - Flags: review+
Attachment #8412142 - Flags: feedback?(bugs)
Comment on attachment 8412142 [details] [diff] [review] Step 1: extend MouseEvent interface per the canvas spec No. It is enough to add new stuff to .webidl.
Attachment #8412142 - Flags: feedback?(bugs) → feedback-
(In reply to Olli Pettay [:smaug] from comment #27) > Comment on attachment 8412142 [details] [diff] [review] > Step 1: extend MouseEvent interface per the canvas spec > > No. It is enough to add new stuff to .webidl. the code does not compile without the change to nsIDOMMouseEvent. Am I. Supposed to make another change?
Why it does not? You need to add the right method declaration to MouseEvent.h and definition to MouseEvent.cpp
Attachment #8412142 - Attachment is obsolete: true
Attachment #8413277 - Flags: review?(bugs)
Attachment #8413068 - Attachment is obsolete: true
Attachment #8413068 - Flags: review?(roc)
Attachment #8413278 - Flags: review?(roc)
(In reply to Olli Pettay [:smaug] from comment #29) > Why it does not? You need to add the right method declaration to > MouseEvent.h and definition to MouseEvent.cpp OK. I updated the patch. It seems a bit strange since it's the only property that is not in the .idl file.
Comment on attachment 8413277 [details] [diff] [review] Step 1: extend MouseEvent interface per the canvas spec Just make GetRegion return void. WebIDL isn't using the return value anyway. r=me with that > it's the only property that is not in the .idl file. The .idl is deprecated, and this is the first property we added since deprecating it.
Attachment #8413277 - Flags: review?(bugs) → review+
(In reply to Boris Zbarsky [:bz] (on PTO, reviews are slow) from comment #33) > Comment on attachment 8413277 [details] [diff] [review] > Step 1: extend MouseEvent interface per the canvas spec > > Just make GetRegion return void. WebIDL isn't using the return value anyway. > > r=me with that Will do! > > > it's the only property that is not in the .idl file. > > The .idl is deprecated, and this is the first property we added since > deprecating it. I see. That makes sense.
Attachment #8413277 - Attachment is obsolete: true
Attachment #8413278 - Attachment is obsolete: true
Attachment #8413278 - Flags: review?(roc)
Attachment #8413307 - Flags: review?(roc)
Fixed issue that was found by the try bots
Attachment #8413307 - Attachment is obsolete: true
Attachment #8413307 - Flags: review?(roc)
Attachment #8413313 - Flags: review?(roc)
Comment on attachment 8412781 [details] [diff] [review] Step 2: Fix hitregion support to keep order of regions. Review of attachment 8412781 [details] [diff] [review]: ----------------------------------------------------------------- RegionInfo looks memmovable to me.
Attachment #8412781 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #39) > Comment on attachment 8412781 [details] [diff] [review] > Step 2: Fix hitregion support to keep order of regions. > > Review of attachment 8412781 [details] [diff] [review]: > ----------------------------------------------------------------- > > RegionInfo looks memmovable to me. That's scary! :-) I think I tried already this and ran into issues/crashes. I will make the change and report back.
Attachment #8412781 - Attachment is obsolete: true
Attachment #8413313 - Attachment is obsolete: true
Attachment #8413313 - Flags: review?(roc)
Attachment #8417791 - Flags: review?(roc)
Attachment #8417792 - Flags: review?(roc)
(In reply to Rik Cabanier from comment #40) > > That's scary! :-) > I think I tried already this and ran into issues/crashes. I will make the > change and report back. well, I made it an object, tested it and there were no issues. I updated the patches.
Marc Sadecki from W3C created a demo for hit regions but it only works on a private build. Can you review my patches so we can land it in nightly soon?
Flags: needinfo?(roc)
Comment on attachment 8417791 [details] [diff] [review] Step 2: Fix hitregion support to keep order of regions. Review of attachment 8417791 [details] [diff] [review]: ----------------------------------------------------------------- Sorry about the delay. ::: content/canvas/src/CanvasRenderingContext2D.cpp @@ +622,5 @@ > mTarget = nullptr; > mStream = nullptr; > > // reset hit regions > + for(unsigned int x = 0 ; x < mHitRegionsOptions.Length(); x++) { space before ( but no space before ; @@ +628,3 @@ > #ifdef ACCESSIBILITY > + if (info.mElement) { > + info.mElement->DeleteProperty(nsGkAtoms::hitregion); Shouldn't we be checking whether info.mElement is a descendant of the canvas? There's actually a bit of a problem here. Suppose we add an element as a descendant of the canvas and add a hitregion property for it. Then we move the element to be the descendant of another canvas and that canvas adds a hitregion property for it, overwriting the first property value ... now both canvases contain this element in their mHitRegionOptions. Then suppose the first canvas clears its list. Here it will delete the hitregion property added by the second canvas, which is wrong. I think the right thing to do is not to attach a hitregion property to elements at all. Instead, when looking for a hitregion, scan the element's ancestors for a canvas, and ask that canvas if it has a hitregion for that element. @@ +2466,5 @@ > > void > CanvasRenderingContext2D::RemoveHitRegion(const nsAString& id) > { > + for(unsigned int x = 0 ; x < mHitRegionsOptions.Length(); x++) { Space before (, but no space before ;
Attachment #8417791 - Flags: review?(roc) → review-
Comment on attachment 8412981 [details] [diff] [review] Step 4: added test for hit regions Review of attachment 8412981 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/test/test_hitregion_event.html @@ +18,5 @@ > +<script type="application/javascript"> > +SpecialPowers.setBoolPref("canvas.hitregions.enabled", true); > + > +SimpleTest.waitForExplicitFinish(); > +SimpleTest.waitForFocus(runTests); Why do you need to wait for focus? @@ +71,5 @@ > +} > + > +/* > +function doTest(aButton) > +{ Why is this commented out? Just take it out of the test.
Attachment #8412981 - Flags: review?(roc) → review-
Comment on attachment 8417791 [details] [diff] [review] Step 2: Fix hitregion support to keep order of regions. Review of attachment 8417791 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/CanvasRenderingContext2D.cpp @@ +628,3 @@ > #ifdef ACCESSIBILITY > + if (info.mElement) { > + info.mElement->DeleteProperty(nsGkAtoms::hitregion); We talked about this when doing the original patch that added hit regions. If an element is moved to a different canvas (or somewhere else in the tree), it's associated hit region data is not cleared. I was going to address that in a different patch but didn't start it yet.
Comment on attachment 8417791 [details] [diff] [review] Step 2: Fix hitregion support to keep order of regions. Review of attachment 8417791 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/CanvasRenderingContext2D.cpp @@ +628,3 @@ > #ifdef ACCESSIBILITY > + if (info.mElement) { > + info.mElement->DeleteProperty(nsGkAtoms::hitregion); I was thinking about your proposal and like it much better. I will look into updating the existing implementation to search for a canvas ancestor.
Attachment #8417791 - Attachment is obsolete: true
Attachment #8412981 - Attachment is obsolete: true
Attachment #8421333 - Attachment is obsolete: true
Attachment #8417792 - Attachment is obsolete: true
Attachment #8421334 - Attachment is patch: true
Attachment #8421339 - Attachment is patch: true
Attachment #8421334 - Attachment is obsolete: true
Attachment #8421341 - Attachment is patch: true
Attachment #8421338 - Flags: review?(roc)
Attachment #8421341 - Flags: review?(roc)
Comment on attachment 8421338 [details] [diff] [review] Step 2: Fix hitregion support to keep order of regions. Review of attachment 8421338 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/generic/Accessible.cpp @@ +923,5 @@ > nsIFrame* frame = GetFrame(); > if (frame && mContent) { > + bool* hasHitRegionRect = static_cast<bool*>(mContent->GetProperty(nsGkAtoms::hitregion)); > + > + if (hasHitRegionRect && mContent->IsElement()) { I kept this variable as an optimization. Otherwise, every time an accessible object is asked for its bounding rect, the inner code is executed.
Attachment #8421341 - Attachment is obsolete: true
Attachment #8421341 - Flags: review?(roc)
Attachment #8421528 - Flags: review?(roc)
Comment on attachment 8421338 [details] [diff] [review] Step 2: Fix hitregion support to keep order of regions. Review of attachment 8421338 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/generic/Accessible.cpp @@ +928,5 @@ > // This is for canvas fallback content > // Find a canvas frame the found hit region is relative to. > nsIFrame* canvasFrame = frame->GetParent(); > while (canvasFrame && (canvasFrame->GetType() != nsGkAtoms::HTMLCanvasFrame)) > canvasFrame = canvasFrame->GetParent(); call nsLayoutUtils::GetClosestFrameOfType @@ +937,5 @@ > + dom::HTMLCanvasElement *canvas = > + dom::HTMLCanvasElement::FromContent(canvasFrame->GetContent()); > + > + // get the bounding rect of the hit region > + if(canvas && canvas->CountContexts() Space before ( @@ +938,5 @@ > + dom::HTMLCanvasElement::FromContent(canvasFrame->GetContent()); > + > + // get the bounding rect of the hit region > + if(canvas && canvas->CountContexts() > + && canvas->GetContextAtIndex(0)->GetHitRegionRect(mContent->AsElement(), aTotalBounds)) { && goes on previous line ::: content/canvas/src/CanvasRenderingContext2D.cpp @@ +622,5 @@ > mTarget = nullptr; > mStream = nullptr; > > // reset hit regions > + for(unsigned int x = 0 ; x < mHitRegionsOptions.Length(); x++) { Space before (, no space before ; @@ +2465,5 @@ > + RemoveHitRegion(options.mId); > + > + bool isDescendant = false; > + > + if (options.mControl != NULL) { nullptr, or just write "if (options.mControl)" @@ +2489,3 @@ > options.mControl->DeleteProperty(nsGkAtoms::hitregion); > + options.mControl->SetProperty(nsGkAtoms::hitregion, new bool(true), > + nsINode::DeleteProperty<bool>); I think we don't need to check isDescendant since that's a rare error case and your new accessibility code will do the right thing if we just set the property unconditionally. Also you don't need to delete the property just before you set it again. @@ +2509,2 @@ > return; > } Don't bother with this length check, there's no performance effect @@ +2509,4 @@ > return; > } > > + for(unsigned int x = 0 ; x < mHitRegionsOptions.Length(); x++) { space before (, not before ; @@ +2519,2 @@ > #endif > + mHitRegionsOptions.RemoveElementAt(x); I don't think you should remove the property. It might not be correct, e.g. if the element is not a descendant of this canvas, we might be removing the property even though it still has a valid hit region in another canvas. Leaving unnecessary properties around might slow down accessibility a little bit but not enough to matter. I actually think you shouldn't remove the property anywhere, since I think it leads to bugs in complex situations (e.g. where elements have moved to be descendants of other canvases) and is not necessary. @@ +2526,5 @@ > + > +bool > +CanvasRenderingContext2D::GetHitRegionRect(Element* aElement, nsRect& aRect) > +{ > + for(unsigned int x = 0 ; x < mHitRegionsOptions.Length(); x++) { space before (, not before ;
Attachment #8421338 - Flags: review?(roc) → review-
Comment on attachment 8421528 [details] [diff] [review] Step 4: added test for hit regions Review of attachment 8421528 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/test/test_hitregion_event.html @@ +15,5 @@ > + > +</div> > +<pre id="test"> > +<script type="application/javascript"> > +SpecialPowers.setBoolPref("canvas.hitregions.enabled", true); call SpecialPowers.pushPrefEnv @@ +18,5 @@ > +<script type="application/javascript"> > +SpecialPowers.setBoolPref("canvas.hitregions.enabled", true); > + > +SimpleTest.waitForExplicitFinish(); > +SimpleTest.waitForFocus(runTests); Again, why do we need focus here? @@ +61,5 @@ > + ctx.removeHitRegion("c"); > + synthesizeMouse(input, 85,65, {type: "mousedown"}); > + is(regionId, "b", "Hit region b", ". Found: " + regionId); > + > + }catch(e) Indent everything inside the try block. Space after } and before (
Attachment #8421528 - Flags: review?(roc) → review-
Comment on attachment 8421528 [details] [diff] [review] Step 4: added test for hit regions Review of attachment 8421528 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/test/test_hitregion_event.html @@ +18,5 @@ > +<script type="application/javascript"> > +SpecialPowers.setBoolPref("canvas.hitregions.enabled", true); > + > +SimpleTest.waitForExplicitFinish(); > +SimpleTest.waitForFocus(runTests); If we don't do this, the test will hang indefinitely. see: https://tbpl.mozilla.org/?tree=Try&rev=0aa9fd9a867c
Comment on attachment 8421338 [details] [diff] [review] Step 2: Fix hitregion support to keep order of regions. Review of attachment 8421338 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/CanvasRenderingContext2D.cpp @@ +2489,3 @@ > options.mControl->DeleteProperty(nsGkAtoms::hitregion); > + options.mControl->SetProperty(nsGkAtoms::hitregion, new bool(true), > + nsINode::DeleteProperty<bool>); true. Done. @@ +2509,2 @@ > return; > } A hit region can have no id. (Instead the control is what makes it unique) This function is only supposed to removed regions that have an id. If I don't make this check and the empty string is passed in, the first region with no id is removed. @@ +2519,2 @@ > #endif > + mHitRegionsOptions.RemoveElementAt(x); true. When the element is deleted, it should automatically delete the property.
Attachment #8421338 - Attachment is obsolete: true
Attachment #8422092 - Flags: review?(roc)
Attachment #8422092 - Attachment is obsolete: true
Attachment #8422092 - Flags: review?(roc)
Attachment #8422110 - Flags: review?(roc)
Comment on attachment 8421528 [details] [diff] [review] Step 4: added test for hit regions Review of attachment 8421528 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/test/test_hitregion_event.html @@ +18,5 @@ > +<script type="application/javascript"> > +SpecialPowers.setBoolPref("canvas.hitregions.enabled", true); > + > +SimpleTest.waitForExplicitFinish(); > +SimpleTest.waitForFocus(runTests); I don't know what I was thinking. Will fix!
Attachment #8421528 - Attachment is obsolete: true
Attachment #8422130 - Flags: review?(roc)
Comment on attachment 8422110 [details] [diff] [review] Step 2: Fix hitregion support to keep order of regions. Review of attachment 8422110 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/CanvasRenderingContext2D.cpp @@ +2483,5 @@ > > void > CanvasRenderingContext2D::RemoveHitRegion(const nsAString& id) > { > + if(id.Length() == 0) { Space before (
Attachment #8422110 - Flags: review?(roc) → review+
Comment on attachment 8422130 [details] [diff] [review] Step 4: added test for hit regions Review of attachment 8422130 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/test/test_hitregion_event.html @@ +63,5 @@ > + } > + > + SimpleTest.finish(); > + } > + trailing whitespace
Attachment #8422130 - Flags: review?(roc) → review+
Attachment #8421339 - Attachment is obsolete: true
Attachment #8422130 - Attachment is obsolete: true
Attachment #8422110 - Attachment is obsolete: true
Attachment #8421339 - Attachment is obsolete: false
Flags: needinfo?(roc)
Keywords: checkin-needed
Also: 3091 INFO TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/webgl-mochitest/test_webgl_available.html | Expected WebGL creation to succeed. https://tbpl.mozilla.org/php/getParsedLog.php?id=39951920&tree=Mozilla-Inbound
The step 3 patch will trigger warnings on every mouse move over a WebGL canvas, no? Why is that desirable, exactly? Worse yet, a mouse move over a canvas that hasn't had any context created yet will make it impossible to create a WebGL context for that canvas in the future. Does the spec really require this behavior (unlikely), or is the patch just not doing what the spec says to do by using getContext (which has side-effects!) when it wants to do something that has no side-effects? Also, mHitRegionsOptions holds strong references to elements but we're not declaring it to cycle collection, it looks like. Why not?
Flags: needinfo?(cabanier)
(In reply to Boris Zbarsky [:bz] from comment #73) > The step 3 patch will trigger warnings on every mouse move over a WebGL > canvas, no? Why is that desirable, exactly? That is not desirable. Does getContext create a context? If so, that is a bug. > Worse yet, a mouse move over a canvas that hasn't had any context created > yet will make it impossible to create a WebGL context for that canvas in the > future. > > Does the spec really require this behavior (unlikely), or is the patch just > not doing what the spec says to do by using getContext (which has > side-effects!) when it wants to do something that has no side-effects? That's a bug. I will fix this. > Also, mHitRegionsOptions holds strong references to elements but we're not > declaring it to cycle collection, it looks like. Why not? Can you tell me how I can work around this? It seems that the lifetime of those elements should be OK since the canvas element would be released and that would also release those elements.
Flags: needinfo?(cabanier)
> Does getContext create a context? Of course. That's its whole purpose. > Can you tell me how I can work around this? Teach the traverse and unlink methods on the canvas context about those strong references. > since the canvas element would be released and that would also release those elements. Those elements can be holding references to the canvas element, no?
(In reply to Boris Zbarsky [:bz] from comment #75) > > Does getContext create a context? > > Of course. That's its whole purpose. oops. I didn't know it was that high level. will fix. > > > Can you tell me how I can work around this? > > Teach the traverse and unlink methods on the canvas context about those > strong references. Can you point me to an example where that is done? > > since the canvas element would be released and that would also release those elements. > > Those elements can be holding references to the canvas element, no? yes but that would be a strange use :-)
> Can you point me to an example where that is done? http://hg.mozilla.org/mozilla-central/file/41a54c8add09/content/canvas/src/CanvasRenderingContext2D.cpp#l478 > yes but that would be a strange use :-) Um. Whether they hold references to the canvas element or not is something the scripts on the page can control, no? Note that these patches got backed out because of leaks, so this is not a hypothetical issue.
Attachment #8424549 - Attachment is obsolete: true
Attachment #8425070 - Flags: review?(bzbarsky)
Attachment #8425070 - Attachment is obsolete: true
Attachment #8425070 - Flags: review?(bzbarsky)
Attachment #8421339 - Attachment is obsolete: true
Comment on attachment 8425072 [details] [diff] [review] Step 3: detect hit on region + fill in region attribute Review of attachment 8425072 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/public/nsICanvasRenderingContextInternal.h @@ +115,5 @@ > // return true and fills in the bounding rect if elementis a child and has a hit region. > virtual bool GetHitRegionRect(mozilla::dom::Element* aElement, nsRect& aRect) { return false; } > > + // Given a point, return hit region ID if it exists or an empty string if it doesn't > + virtual nsString GetHitRegion(const mozilla::gfx::Point& aPoint) { return nsString(); } Added a new method that is implemented on all canvas contexts. ::: content/canvas/src/CanvasRenderingContext2D.cpp @@ +487,5 @@ > + RegionInfo& info = tmp->mHitRegionsOptions[x]; > + if (info.mElement) { > + ImplCycleCollectionUnlink(info.mElement); > + } > + } new per your suggestion ::: content/html/content/src/HTMLCanvasElement.cpp @@ +295,5 @@ > +nsresult HTMLCanvasElement::PreHandleEvent(EventChainPreVisitor& aVisitor) > +{ > + if (aVisitor.mEvent->eventStructType == NS_MOUSE_EVENT) { > + WidgetMouseEventBase* evt = (WidgetMouseEventBase*)aVisitor.mEvent; > + if (mCurrentContext) { no more getContext. This will now happen on every type of canvas element.
Attachment #8425072 - Flags: review?(bzbarsky)
Attachment #8424549 - Attachment is obsolete: false
I promised some people from w3c that this could land soon. Can you take a look? The memory leak was gone with the change to the CYCLE stuff.
Flags: needinfo?(bzbarsky)
Comment on attachment 8425072 [details] [diff] [review] Step 3: detect hit on region + fill in region attribute >+ for (unsigned int x = 0 ; x < tmp->mHitRegionsOptions.Length(); x++) { size_t, not unsigned int. >+ if (info.mElement) { ImplCycleCollectionUnlink and ImplCycleCollectionTraverse are both null-safe, fwiw. I assume the slightly odd return value setup for GetHitRegion already has review, but it should also use size_t for the loop variable. The change to PreHandleEvent needs review from someone who knows what this code is trying to do, which is not me. r=me on the CC bits, in any case.
Attachment #8425072 - Flags: review?(bzbarsky) → review+
Attachment #8425072 - Attachment is obsolete: true
Comment on attachment 8425283 [details] [diff] [review] Step 3: detect hit on region + fill in region attribute Review of attachment 8425283 [details] [diff] [review]: ----------------------------------------------------------------- Sorry to send this back to you one last time. bz found an issue with the previous code. ::: content/canvas/public/nsICanvasRenderingContextInternal.h @@ +115,5 @@ > // return true and fills in the bounding rect if elementis a child and has a hit region. > virtual bool GetHitRegionRect(mozilla::dom::Element* aElement, nsRect& aRect) { return false; } > > + // Given a point, return hit region ID if it exists or an empty string if it doesn't > + virtual nsString GetHitRegion(const mozilla::gfx::Point& aPoint) { return nsString(); } new generic method on all contexts. ::: content/html/content/src/HTMLCanvasElement.cpp @@ +295,5 @@ > +nsresult HTMLCanvasElement::PreHandleEvent(EventChainPreVisitor& aVisitor) > +{ > + if (aVisitor.mEvent->eventStructType == NS_MOUSE_EVENT) { > + WidgetMouseEventBase* evt = (WidgetMouseEventBase*)aVisitor.mEvent; > + if (mCurrentContext) { Code was changed so all contexts have a GetHitRegion virtual method. That way, we don't have to special case 2d canvas contexts here.
Attachment #8425283 - Flags: review?(roc)
Flags: needinfo?(bzbarsky)
Can you review this? It is a small change and I added comments where they were made.
Flags: needinfo?(roc)
Flags: needinfo?(roc)
Keywords: checkin-needed
Is there a Try run showing comment 72 as being fixed with the current patches?
Keywords: checkin-needed
Do you want me to run on with all the platforms?
I remembered I did one with all the platform: https://tbpl.mozilla.org/?tree=Try&rev=94627b70d6af
Keywords: checkin-needed
Adding dev-doc-needed as it extends MouseEvent and this has to be documented.
Keywords: dev-doc-needed
(In reply to Jean-Yves Perrier [:teoli] from comment #94) > One page still need to be created to call this documented: > https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent.region Done.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: