Closed Bug 489127 Opened 15 years ago Closed 14 years ago

nodesFromRect required for better usability on mobile devices

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
Other
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.3a5
Tracking Flags:
Tracking Status
status1.9.2 --- .5-fixed

People

(Reporter: romaxa, Assigned: Felipe)

References

Details

(Keywords: dev-doc-complete)

Attachments

(10 files, 21 obsolete files)

Page for some tests
982 bytes, text/html
Details
Example of not optimal result
7.75 KB, image/png
Details
Part 1 - main nsDisplayList::HitTest change, plus the simple HitTests (v3)
16.83 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 2 - the non-simple HitTest changes (v3)
17.67 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 3 - Add nsLayoutUtils::GetFramesForArea and make GetFrameForPoint a wrapper for it (v3)
4.36 KB, patch
roc
: review+
Details | Diff | Splinter Review
Parts 1 and 2 merged for check-in
34.35 KB, patch
Details | Diff | Splinter Review
Part 4 - for check-in (with float args)
11.61 KB, patch
Details | Diff | Splinter Review
Part 5 - tests (with float args)
8.89 KB, patch
Details | Diff | Splinter Review
Patch for 1.9.2
62.74 KB, patch
roc
: review+
pavlov
: approval1.9.2.5+
Details | Diff | Splinter Review
fix and reenables test
14.69 KB, patch
Details | Diff | Splinter Review 
On mobile devices we have small screen, small links e.t.c..
In most cases it is very hard to click on link if that is very small and user clicking with finger.
I'm not sure what is the best solution for it, but I think layout should provide some API for sending events with area, or get list of elements for some area and allow for application (fennec) chose best element in that area.
Blocks: 441585
Fingertouch, an experimental project of Opera.
http://labs.opera.com/news/2009/03/05/
In the demo video, links around touched point are zoomed and highlighted.
Attached patch WIP proposal for internal API. (obsolete) — — Splinter Review 
I'm not sure, would it be better to merge ElementFromPoint function into this
Area function...
Attachment #394163 - Flags: review?(roc)
(In reply to comment #1)
> Fingertouch, an experimental project of Opera.
> http://labs.opera.com/news/2009/03/05/
> In the demo video, links around touched point are zoomed and highlighted.

Can I see implementation? ;)
roc, can you check this patch?
+PresShell::GetNodesForArea(nsRect aRect, nsISupportsArray **aOutNodes,

This should be nsIntRect

nsLayoutUtils::GetFramesForArea should set IsForEventHandling on the builder.

I think what you really need here is to change the signature of HitTest to take a rectangle parameter instead of a point. It would return true whenever the "hit area" of the display item intersects the rectangle.
(In reply to comment #5)
> +PresShell::GetNodesForArea(nsRect aRect, nsISupportsArray **aOutNodes,
> 
> This should be nsIntRect
> 
> nsLayoutUtils::GetFramesForArea should set IsForEventHandling on the builder.

What do you mean here "IsForEventHandling", I found only "IsForEventDelivery"... and I'm setting it as TRUE in 

+  nsDisplayListBuilder builder(aFrame, PR_TRUE, PR_FALSE);
+  nsDisplayList list;
> I think what you really need here is to change the signature of HitTest to take
> a rectangle parameter instead of a point. It would return true whenever the
> "hit area" of the display item intersects the rectangle.

How it will help for current implementation? what is the point of changing HitTest API?

Also how can I get list of frames in this case? should I modify current DisplayList and remove items which are passing HitTest?
(In reply to comment #8)
> > I think what you really need here is to change the signature of HitTest to take
> > a rectangle parameter instead of a point. It would return true whenever the
> > "hit area" of the display item intersects the rectangle.
> 
> How it will help for current implementation? what is the point of changing
> HitTest API?

The goal is to find all frames that would accept a mouse event anywhere in given rectangle, right?

> Also how can I get list of frames in this case? should I modify current
> DisplayList and remove items which are passing HitTest?

You don't need to modify it, you can just scan it.
I think I just don't understand what you mean...
If I modify HiTest API for DisplayList and DisplayItem, then I should modify also significant part of layout.

which HitTest you are talking about? DisplayList or DisplayItem? or both?
Here is the draft implementation by using new HitTest function (Rect based).
roc can you check nsDisplayList::HitTest new implementation, and give some comments about it?
Attachment #394163 - Attachment is obsolete: true
Attachment #408387 - Flags: review?(roc)
Attachment #394163 - Flags: review?(roc)
HitTest should take a const nsRect& instead of just nsRect, so we don't have to copy the rect a lot.

-nsLayoutUtils::GetFramesForArea(nsIFrame* aFrame, nsRect aRect,

This patch is on top of your other patch?

+  // Create region for handling already occupied area
+  nsRegion workRegion(aRect);

If you want this to really work properly then you need to pass nsRegion as a parameter to HitTest. But I don't think we really need to exclude frames covered by other frames from the list. Why can't you just return all frames whose hit-regions intersect the given rect, and then just use the topmost one, or sort them out however you want? How exactly are you going to use the list of frames?

It looks like you're just using the top-left of the rectangle in many of the HitTest method implementations; you probably need to modify them to consider the whole rectangle.
(In reply to comment #12)
> HitTest should take a const nsRect& instead of just nsRect, so we don't have to copy the rect a lot.

Ok,

> 
> -nsLayoutUtils::GetFramesForArea(nsIFrame* aFrame, nsRect aRect,
> 
> This patch is on top of your other patch?

Yep, I was wanted to check that I'm not going in completely wrong direction

> 
> +  // Create region for handling already occupied area
> +  nsRegion workRegion(aRect);
> 
> If you want this to really work properly then you need to pass nsRegion as a
> parameter to HitTest. But I don't think we really need to exclude frames
> covered by other frames from the list. Why can't you just return all frames
> whose hit-regions intersect the given rect, and then just use the topmost one,
> or sort them out however you want? How exactly are you going to use the list of
> frames?

Yes, I think I can keep all frames in that list, but I though that it may confuse a bit, because for this test:
'data:text/html,<input style="position:absolute;top:100px;left:200px;width:100px;height:50px;"><input type=submit style="position:absolute;top:100px;left:300px;width:100px;height:50px;">'

When we are hovering mouse on "input" or button field, it including always "ScrollFrame" and "CanvasFrame" in that list.

If it is ok, then I can keep them in list, and take only top frame.

> 
> It looks like you're just using the top-left of the rectangle in many of the
> HitTest method implementations; you probably need to modify them to consider
> the whole rectangle.

Yes, that part is not finished yet, I just convert it quickly to new API. I will try to create better migrate to new API.

Also should I keep old function nsLayoutUtils::GetFrameForPoint and make it as wrapper for nsLayoutUtils::GetFramesForArea? in this case I can keep some "content" parts without changes.
Keep nsLayoutUtils::GetFrameForPoint.
> Also should I keep old function nsLayoutUtils::GetFrameForPoint and make it as
> wrapper for nsLayoutUtils::GetFramesForArea?

Yes.

I'm actually not sure how you're going to deal with arbitrary 2D transforms here. I suppose you can transform your rectangle into its bounding-box in the transformed element and continue with that. The results won't be perfect but they'll be good enough.
Comment on attachment 408387 [details] [diff] [review]
Test patch with new HitTest function

comments need to be addressed
Attachment #408387 - Flags: review?(roc) → review-
Oleg,

What rect would an app pass into the function? We started work on a different approach for hit testing elements. We use CSS to increase the bounding box area of certain elements and then use the larger bounding box when hit testing.

The bounding box could increased on each side separately. We'll start putting patches in bug 547997.
(In reply to comment #17)
> Oleg,
> 
> What rect would an app pass into the function? We started work on a different

Rect is supposed to be a click point + radius which is taken from pressure value, or capacitive touch screen area
Attached patch WIP v2 - new hittest (obsolete) — — Splinter Review 
Today I made some progress with this patch. I haven't addressed all the comments yet but it's working ok. I kept GetFrameForPoint as a wrapper for GetFramesForArea, and removed the workRegion part so we get all the frames in the area, regardless if they overlap or not.  Also, I built a faster path for the single point case, keeping it closer to as if there was no change.

(ignore my printfs on the patch for now)

I guess to proceed now I need some input on how we will want to use these changes. Talking with vingtetun and mfinkle on #mobile it is clearer to me now, but I still wanna point this out in here so we know the rationale.

It looks like that for Fennec we want these results in JS, and for Firefox in C++. The conversion from nsIFrame to nsIDOMElement element happens in nsDocument::elementFromPoint.
I think we could keep elementFromPoint intact, and create a new one that returns an nsIDOMNodeList and be used only in the specific places we want to use it.
(As correctly suggested by the title of this bug)


Anyway, it looks like that picking the first element on the list is almost always the right one [note: *almost*. I have a test case that it's not the right one]. If we keep with an array, what other info will we need to have to pick up the right element (assuming we don't want to always pick the first one), and how will we have these values in C++ and JS.
Assignee: nobody → felipc
Attachment #408387 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attached file Page for some tests — 
I'm using this page to test the new hit targets by seeing the :hover behavior.  In the final version this won't work because we'll probably only adjust clicks
From the test page above, these are the current hit areas for the links when I expand the rect by 10px to each side, and always pick the first frame in the list.

The colored areas represent each link, and the gray area represent the " / " text node.

Note that the target areas of the " / " text win over the links on the left side. I don't think this is a bug though. Originally I'd think that the text nodes of the links, being inside the <a>, would be on top of the plain text.  But it looks like they have the same ordering and are just stacked from first to last.  roc, is this understanding right?

On the list of potential targets, when I hover on the gray areas I have the " / " text node as 1st, the "Link" text node as 2nd, the <a> element as 3rd, <p> is 4th, <body> 5th, <html> 6th.


If that is correct, one way to solve this would be to check if a text node is associated with an <a> element, or how deep each one is on the tree, or any other reasoning.
That's right, the default z-order is simply DOM order. So given <a>Foo</a>Bar, Bar is logically above Foo in z-order.
Ok, this patch now adds elementsFromArea to DOMWindowUtils, similar to elementFromPoint. It's not perfect yet but should be useful if anyone wants to apply it locally and play with it from JS.

There's a difference from elementFromPoint though: when elementFromPoint finds a text node, it walks up to the parent to always return a DOMElement, whereas here in elementsFromArea we want to have the actual DOMNode on the list returned.


roc, I'm still a little confused by the difference between nsRect and nsIntRect. On comment 5 you requested it to be nsIntRect.  Can you explain what the basic difference should be?  Or is it just a s/nsRect/nsIntRect/g ?


The patch is still rough but I think now we'll be able to start thinking about the disambiguation strategies (which should be done in another bug). From the testing that I've been doing, it looks like the hardest thing will be to decide between sibling elements.
Attachment #433238 - Attachment is obsolete: true
If the rect is in CSS pixels or device pixels, it should be an nsIntRect. It should only be an nsRect if it's in appunits.

If you're going to report text nodes, call it NodeFromPointWithArea.
Attached patch WIP v4 (obsolete) — — Splinter Review 
Okay, got more progress here. I've implemented most of the HitTests now, just missing the SVG ones. Now the XUL menus work again! I did what was suggested in comment #15 about the 2d transforms.

Before I continue on the patch, just some questions and comments for anyone who wants to chime in

> If the rect is in CSS pixels or device pixels, it should be an nsIntRect. It
> should only be an nsRect if it's in appunits.
Ah ok, thanks! All rects here are in appunits, so none will have to be changed.
 
> If you're going to report text nodes, call it NodeFromPointWithArea.
Sure. Just thinking about it more, stechz mentioned on the phone that the input here is just a rect, not an arbitrary area.  So perhaps we could make it NodesFromRect ?  Sounds simpler too.


Do we want this exposed on nsIDocument too, like elementFromPoint? Or do we keep it only for nsIDOMWindowUtils?


I believe that the rect size parameters should be in device pixels, not css pixels. If we are zoomed 8x, I don't think we want to scan an area 8x bigger. We can leave it to the caller to decide.


I'm just a little concerned if this will introduce any perf problems (because we're changing from returning a pointer to building arrays), considering how HitTest is called a lot of times during mouse movements. If after I get some try builds out of it we see perf regressions we will probably need an approach that doesn't change HitTest, for ex. creating an alternate AreaTest path. 



I'm posting the updated patch just for what it's worth (I haven't checked if it compiles). But when it gets to review phase I'm gonna break it in various chunks from the patch queue. Also, this will need a bajilion tests, so I'm gonna start writing some soon.
(In reply to comment #26)
> > If the rect is in CSS pixels or device pixels, it should be an nsIntRect. It
> > should only be an nsRect if it's in appunits.
> Ah ok, thanks! All rects here are in appunits, so none will have to be
> changed.

OK

> Sure. Just thinking about it more, stechz mentioned on the phone that the
> input here is just a rect, not an arbitrary area.  So perhaps we could make it
> NodesFromRect ?  Sounds simpler too.

Works for me.

> Do we want this exposed on nsIDocument too, like elementFromPoint? Or do we
> keep it only for nsIDOMWindowUtils?

The latter, for now.

> I believe that the rect size parameters should be in device pixels, not css
> pixels. If we are zoomed 8x, I don't think we want to scan an area 8x bigger.
> We can leave it to the caller to decide.

Generally I think APIs like this should use CSS pixels so they're resolution independent by default.

> I'm just a little concerned if this will introduce any perf problems (because
> we're changing from returning a pointer to building arrays), considering how
> HitTest is called a lot of times during mouse movements. If after I get some
> try builds out of it we see perf regressions we will probably need an approach
> that doesn't change HitTest, for ex. creating an alternate AreaTest path. 

I don't think it's going to be a problem. Don't worry about it. Building one array per mouse event is totally insignificant.
Attached patch v5 = dwu.nodesFromRect (obsolete) — — Splinter Review 
New update, almost there.  I'm sending this to tryserver now to see how it behaves, and will report back.

Some new considerations:

* I'm treating an SVG element as a final node, meaning that the multiple-frames hittest doesn't go inside the SVG to possibly return multiple glyphs, paths, etc. I don't think we would want that since we're filtering in only eELEMENTs and eTEXTs nodes.

* Using the approach for 2d transformation of using its bounding box to intersect with our rect, it looks to me that this will change the current results even if we pass a rect of the size of a single point.
I'm waiting to see if the tryserver will report any test failure due to this, but maybe we'll have to special case single points to the old precise behavior or use some other approach.

* I mentioned on a previous comment that I built a faster path for single points, by simply returning the first frame it finds, keeping it close to the previous behavior.  But this implies that the hittest function have two behaviors (for single points return only 1 element, for rects return all found), so maybe I should get rid of this special condition?
Attachment #433612 - Attachment is obsolete: true
Attachment #434174 - Attachment is obsolete: true
Comment on attachment 435759 [details] [diff] [review]
v5 = dwu.nodesFromRect

The first version I sent to try server failed some tests, but I've now fixed it and it produced a green run on Linux hg unit test:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1269979828.1269990851.17967.gz

I'll start attaching the patches now.
Attachment #435759 - Attachment is obsolete: true
Attachment #436065 - Flags: review?(roc)
I shamelessly left a "// XXX felipe" comment here to be removed since I couldn't get the right var type ..


oh, and a question about this part:

do we follow the same behavior of elementFromPoint of not returning the elements on subdocuments, but actually returns the corresponding iframe in the main document?

the thing here is that since nodesForRect is not added to |document|, we have to get DOMWindowUtils for the subdocument's window if we want to get elements from it.  This is not a problem though.. just brainstorming
Attachment #436070 - Flags: review?(roc)
just to add one more info, related to part 4 (and 3):
It looks like there's always 3 different frames associated with <html> : Block, Canvas and HTMLScroll frames.. So since here we get all frames and find their related elements/nodes, we end up with <html> 3 times on the end of the list.
Can we leave this as is? or filter out repeated elements?
(In reply to comment #28)
> * I'm treating an SVG element as a final node, meaning that the multiple-frames
> hittest doesn't go inside the SVG to possibly return multiple glyphs, paths,
> etc. I don't think we would want that since we're filtering in only eELEMENTs
> and eTEXTs nodes.

SVG elements are eELEMENTs so we'll want to fix this at some point, but it doesn't need to be now.

> * Using the approach for 2d transformation of using its bounding box to
> intersect with our rect, it looks to me that this will change the current
> results even if we pass a rect of the size of a single point.
> I'm waiting to see if the tryserver will report any test failure due to this,
> but maybe we'll have to special case single points to the old precise behavior
> or use some other approach.

You probably want to special-case a single point so that it remains a single point.

> * I mentioned on a previous comment that I built a faster path for single
> points, by simply returning the first frame it finds, keeping it close to the
> previous behavior.  But this implies that the hittest function have two
> behaviors (for single points return only 1 element, for rects return all
> found), so maybe I should get rid of this special condition?

I think you should get rid of the special behavior. We don't need to optimize here.

(In reply to comment #33)
> do we follow the same behavior of elementFromPoint of not returning the
> elements on subdocuments, but actually returns the corresponding iframe in the
> main document?
>
> the thing here is that since nodesForRect is not added to |document|, we have
> to get DOMWindowUtils for the subdocument's window if we want to get elements
> from it.  This is not a problem though.. just brainstorming

Currently nodesFromRect doesn't require chrome privileges so we should insist that it only return elements from the current document. If you want it to return nodes from arbitrary subdocuments, I think you should add a chrome privilege check in the method like we do for other DOMWindowUtils methods.

(In reply to comment #34)
> just to add one more info, related to part 4 (and 3):
> It looks like there's always 3 different frames associated with <html> :
> Block, Canvas and HTMLScroll frames.. So since here we get all frames and
> find their related elements/nodes, we end up with <html> 3 times on the end
> of the list. Can we leave this as is? or filter out repeated elements?

I think you should filter out repeated elements, otherwise it will be confusing.
+PRBool nsDisplayItem::HitTest(nsDisplayListBuilder *aBuilder,

This doesn't need to return a result now, right? Callers can tell if frame is added to aOutFrames.

+  static nsresult GetFramesForArea(nsIFrame* aFrame, nsRect aRect,

const nsRect&

Make all those changes and I'll re-review properly.
(In reply to comment #35)
> SVG elements are eELEMENTs so we'll want to fix this at some point, but it
> doesn't need to be now.
Ok
 
> You probably want to special-case a single point so that it remains a single
> point.
Alright, I've done this now. FWIW, there were no test failures on the previous version, but I think it's a good thing to do regardless

> I think you should get rid of the special behavior. We don't need to optimize
> here.
Ok, got rid of it

> Currently nodesFromRect doesn't require chrome privileges so we should insist
> that it only return elements from the current document. If you want it to
> return nodes from arbitrary subdocuments, I think you should add a chrome
> privilege check in the method like we do for other DOMWindowUtils methods.
Ok, we don't strictly need returning elements from subdocuments, so I'll keep this just returning elements from the current document

> I think you should filter out repeated elements, otherwise it will be
> confusing.
Right. I kept GetFramesForArea returning everything, since they're in fact different frames (not repeated), and now repetations are filtered out when we convert them from frame to node on domwindowutils. 

(In reply to comment #36)
> +PRBool nsDisplayItem::HitTest(nsDisplayListBuilder *aBuilder,
> 
> This doesn't need to return a result now, right? Callers can tell if frame is
> added to aOutFrames.
>
That's right. And most callers don't care about a result anyway, so I removed the return value and if a caller needs to check hits it see if aOutFrames had anything added

> 
> +  static nsresult GetFramesForArea(nsIFrame* aFrame, nsRect aRect,
> 
> const nsRect&
Ok


Beginning attaching the patches again. I just sent it to tryserver but the only change in behavior is the case for single-point in 2d transforms, so I believe it will run fine.

        Attachment #436064 -
        Attachment is obsolete: true

        Attachment #436263 -
        Flags: review?(roc)

        Attachment #436064 -
        Flags: review?(roc)

    
    

    
  


  

        Attachment #436065 -
        Attachment is obsolete: true

        Attachment #436264 -
        Flags: review?(roc)

        Attachment #436065 -
        Flags: review?(roc)

    
    

    
  


  

        Attachment #436070 -
        Attachment is obsolete: true

        Attachment #436267 -
        Flags: review?(roc)

        Attachment #436070 -
        Flags: review?(roc)

    
    

    
  
(In reply to comment #37)
> (In reply to comment #35)
> > You probably want to special-case a single point so that it remains a single
> > point.
> Alright, I've done this now. FWIW, there were no test failures on the previous
> version, but I think it's a good thing to do regardless

Then you should add a test that does fail without that special case! :-)

> > I think you should filter out repeated elements, otherwise it will be
> > confusing.
> Right. I kept GetFramesForArea returning everything, since they're in fact
> different frames (not repeated), and now repetations are filtered out when we
> convert them from frame to node on domwindowutils.

That sounds great.

    
    

    
  
Hmm, why allow aOutFrames to be null? That just adds a bunch of if statements to HitTest implementations. Let's get rid of that.

+  if (aRect.width == 1 && aRect.height == 1) {
+    gfxPoint point = matrix.Transform(gfxPoint(NSAppUnitsToFloatPixels(aRect.x, factor),
+                                               NSAppUnitsToFloatPixels(aRect.y, factor)));
+
+    resultingRect = nsRect(NSFloatPixelsToAppUnits(float(point.x), factor),
+                           NSFloatPixelsToAppUnits(float(point.y), factor),
+                           NSFloatPixelsToAppUnits(1.0f, factor),
+                           NSFloatPixelsToAppUnits(1.0f, factor));

Here you're expanding a 1-appunit x 1-appunit rect to a 1-pixel x 1-pixel rect. That's not what we want, especially because it means if we encounter another transform this special case isn't going to apply again.

+  if (frame)
+    aOutFrames->AppendElement(frame);

{} around if bodies please (except for return/break/continue)

+  if (aFlushLayout)
+    FlushPendingNotifications(Flush_Layout);

here too

Should nodesFromRect return null or an empty list when there are no nodes? I'm actually not sure. Maybe an empty list is less surprising?

Otherwise looks good!

    
    

    
  
(In reply to comment #43)
> Hmm, why allow aOutFrames to be null? That just adds a bunch of if statements
> to HitTest implementations. Let's get rid of that.

Oh, can I just assume aOutFrames will never be null? (I didn't know the precise behavior of nsTArray, so I went with the explicit checks). In this case, I'll remove the checks and the default param aOutFrames = nsnull from the HitTests.

> 
> +  if (aRect.width == 1 && aRect.height == 1) {
> +    gfxPoint point =
> matrix.Transform(gfxPoint(NSAppUnitsToFloatPixels(aRect.x, factor),
> +                                              
> NSAppUnitsToFloatPixels(aRect.y, factor)));
> +
> +    resultingRect = nsRect(NSFloatPixelsToAppUnits(float(point.x), factor),
> +                           NSFloatPixelsToAppUnits(float(point.y), factor),
> +                           NSFloatPixelsToAppUnits(1.0f, factor),
> +                           NSFloatPixelsToAppUnits(1.0f, factor));
> 
> Here you're expanding a 1-appunit x 1-appunit rect to a 1-pixel x 1-pixel rect.
> That's not what we want, especially because it means if we encounter another
> transform this special case isn't going to apply again.

oops, my bad. It should have been resultingRect = nsRect(..., ..., 1, 1) here

> 
> +  if (frame)
> +    aOutFrames->AppendElement(frame);
> 
> {} around if bodies please (except for return/break/continue)
> 
> +  if (aFlushLayout)
> +    FlushPendingNotifications(Flush_Layout);
> 
> here too
ok

> Should nodesFromRect return null or an empty list when there are no nodes? I'm
> actually not sure. Maybe an empty list is less surprising?

I'd say return an empty list, since the caller will probably follow up with a "for (i = 0; i < results.length; i++)", and then it doesn't need to check for null.

    
    

    
  


  

        Attachment #436264 -
        Attachment is obsolete: true

        Attachment #436393 -
        Flags: review?(roc)

        Attachment #436264 -
        Flags: review?(roc)

    
    

    
  


  

        Attachment #436267 -
        Attachment is obsolete: true

        Attachment #436395 -
        Flags: review?(roc)

        Attachment #436267 -
        Flags: review?(roc)

    
    

    
  
Comment on attachment 436395 [details] [diff] [review]
Part 4 - add DOMWindowUtils.nodesForRect (v3)

+  NS_IF_ADDREF(elements);

Should just be NS_ADDREF(elements) since new can't return null now

        Attachment #436395 -
        Flags: review?(roc) → review+

    
    

    
  
Please combine parts 1 and 2 into a single patch before you commit. That way, the build won't be broken if someone's doing a bisection search.

It's still possible for an element to occur twice in the list with another element in between, although that would be very rare. I think that's OK, but you might want to document it.

    
    

    
  
Also, what about tests? This really needs tests.

    
    

    
  
(In reply to comment #51)
> Also, what about tests? This really needs tests.

I'll follow up with tests for this similar to the ones for elementFromPoint, and will also try to find some tricky test cases for nsDisplayTransform::HitTest

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 4 - updated (obsolete)
        — 
          — Splinter Review
      

    


  
Updated NS_ADDREF and a comment to reflect that only repeated elements on sequence are filtered

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch for 1.9.2 (obsolete)
        — 
          — Splinter Review
      

    


  
We need nodesFromRect for fennec 1.1. I've port the patch from trunk to 1.9.2 but i'm not sure, for the .idl, if i need to use the IID from felipe's patch or use another one.

        Attachment #436465 -
        Flags: review?(roc)

    
    

    
  
Comment on attachment 436465 [details] [diff] [review]
Patch for 1.9.2

Looks OK but Felipe should land his patch on trunk and we'll let it bake for a couple of weeks before we risk it on branch. Ideally Fennec would have its own branch so we don't have to expose Firefox 3.6 users to this risk...

        Attachment #436465 -
        Flags: review?(roc) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 5 - tests (obsolete)
        — 
          — Splinter Review
      

    


  
Tests!

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 4 - for check-in (obsolete)
        — 
          — Splinter Review
      

    


  
Small update to part 4 due to a 1-pixel bug I found when writing the tests.

> nscoord w = nsPresContext::CSSPixelsToAppUnits(aLeftSize + aRightSize);
> nscoord h = nsPresContext::CSSPixelsToAppUnits(aTopSize + aBottomSize);

should have been

> nscoord w = nsPresContext::CSSPixelsToAppUnits(aLeftSize + aRightSize + 1);
> nscoord h = nsPresContext::CSSPixelsToAppUnits(aTopSize + aBottomSize + 1);

        Attachment #436415 -
        Attachment is obsolete: true

    
    

    
  
Can somebody update the summary to match reality?
Summary: ElementFromPointWithArea required for better usability on mobile devices. → nodesFromRect required for better usability on mobile devices

    
    

    
  


  
This weekend, bug 463104 landed making elementFromPoint take floating-point coords, so I updated this patch to make nodesFromRect take floats too for parity. (Also updated test cases for this)

Minor param changes, and we can take the int-params patch if preferred.

        Attachment #436395 -
        Attachment is obsolete: true

        Attachment #436570 -
        Attachment is obsolete: true

    
    

    
  


  
Tests with float args

        Attachment #436567 -
        Attachment is obsolete: true

    
    

    
  
Patch landed but some tests failed on linux due to which I believe to be default font-size differences, since all the incorrect positions happened after the div with the "Link" text nodes on it.

Working to sort it out

    
    

    
  
You could probably rewrite the tests to not use text.

    
  
Depends on: 557987

    
    

    
  

      
      
      
      

        Attached patch
          
          
        fix and reenables test (obsolete)
        — 
          — Splinter Review
      

    


  
Yeah, differences on font-size and font-weight were the problem. I did some reordering of the tests to get rid of all text were I need precise positioning, and since I wanted to keep tests for text nodes I calculate their position instead of trying to assume reasonable values.

I tested this locally on mac and windows and it's working now.  Waiting on tryserver for linux results and I'll poke someone in the morning to land the fix.

    
    

    
  
Tests succeeded on tryserver, we can push the fix and re-enable them now

    
  
Depends on: 558162

    
    

    
  
Comment on attachment 436465 [details] [diff] [review]
Patch for 1.9.2

Looking for approval for the 192 version of this patch. The patch is not maemo-only.

        Attachment #436465 -
        Flags: approval1.9.2.4?

    
    

    
  
the 1.9.2 patch will need some minor updates:
 - comment 58
 - fix from bug 557987

and this 192 patch takes int arguments instead of floating-points but I think that's ok since bug 463104 wasn't backported.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Backport from 1.9.3 (obsolete)
        — 
          — Splinter Review
      

    


  
Backport to 1.9.2 with tests.

        Attachment #436465 -
        Attachment is obsolete: true

        Attachment #436465 -
        Flags: approval1.9.2.4?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch for 1.9.2
        — 
          — Splinter Review
      

    


  
Small updates to include the latest test change I had made.

only differences from what landed on m-c:

- changed NS_ADDREF to NS_IF_ADDREF
- the nodesFromRect function is added now to the nsIDOMWindowUtils_1.9.2 interface  (per bug 544765 comment 10)

        Attachment #438578 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 438589 [details] [diff] [review]
Patch for 1.9.2

forgot to set the r? flag. roc, see last comment (comment 69).

I sent this to tryserver, all tests passed.

        Attachment #438589 -
        Flags: review?(roc)

        Attachment #438589 -
        Flags: approval1.9.2.5?

    
    

    
  
Comment on attachment 438589 [details] [diff] [review]
Patch for 1.9.2

Approval notes: This code is not Maemo only. The patch does include a test.

    
  

        Attachment #438589 -
        Flags: approval1.9.2.5? → approval1.9.2.5+

    
    

    
  
pushed to m-192:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5af70883c1de
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED

    
    

    
  
The only thing left to push here is "fix and reenables test"

    
  
Depends on: 562554

    
    

    
  
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=a1d1e59ac1de

          status1.9.2:
          --- → .5-fixed
Flags: in-testsuite?
Target Milestone: --- → mozilla1.9.3a5

    
    

    
  
This broke various binary extensions on 1.9.2 because it changed nsIDocument IID.
Please backout or add nsIDocument_1_9_2.

    
    

    
  
I filed bug 563245 on the nsIDocument issue so that I could nominate it for blocking and we can track the problem better.

    
    

    
  
(In reply to comment #75)
> This broke various binary extensions on 1.9.2 because it changed nsIDocument
> IID.

It also changed nsIDOMWindowUtils_1_9_2 which we have shipped, which now requires an additional interface. On past branches we used longer/uglier names that lent themselves to sequential names--nsIDOMWindowUtils_1_9_2_BRANCH3, for example--but I guess in this form you can just append a _4 or whatever 1.9.2.x release we're on.)

This is one reason the review guidelines require a sr+ for interface changes, it gives us a second chance to catch problems like this.
http://www.mozilla.org/hacking/reviewers.html

Technically it doesn't appear this code had an appropriate reviewer in the first place, though roc would have worked for the sr. Maybe there's a standing delegation from the current module owners that covers it? If so we should add roc to the published peers list for DOM/Content
http://www.mozilla.org/about/owners.html

    
    

    
  
Sorry, I broke all kinds of rules here. Most of the code was layout code and I forgot to get extra review on the content code. And I also forgot to get sr for the interface changes. Just mistakes. Sorry.

    
    

    
  
(Would be nice to have some kind of automatic system for detecting such changes on the branch and making Tinderbox burn!)

    
    

    
  
I can't see what needs to be checked in here, so removing checkin-needed.
Keywords: checkin-needed

    
    

    
  
The "fix and reenables tests" is the only patch that still needs to be checked in. We temporarily disabled them because there were those text-size differences on platforms. FWIW, the test is already fixed and enabled in 1.9.2. It's just missing from m-c.

    
    

    
  
The patch doesn't apply cleanly for me, but I can't see why.

    
    

    
  


  
updated to tip

        Attachment #437766 -
        Attachment is obsolete: true
Flags: in-testsuite? → in-testsuite+

    
  
Keywords: dev-doc-needed

    
    

    
  
it'd be good to update HitTest methods' headers touched here. For example:

in layout/base/nsDisplayList.h

/**
 * This is called after we've constructed a display list for event handling.
 * When this is called, we've already ensured that aPt is in the item's bounds.
 * 
 * @param aState must point to a HitTestState. If you don't have one,
 * just create one with the default constructor and pass it in.
 * @return the frame that the point is considered over, or nsnull if
 * this is not over any frame
 */
virtual void HitTest(nsDisplayListBuilder* aBuilder, const nsRect& aRect,
                     HitTestState* aState, nsTArray<nsIFrame*> *aOutFrames) {}

header mentions, aPt and a return value...

    
    

    
  
My cleanup patches in bug 563878 do this. Feel free to steal from them as they won't be landing on 1.9.2.
Depends on: 561243
Depends on: 731293

    
    

    
  
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c0f887d8cf11
No bug - Fix newlines in dom/tests/mochitest/chrome/489127.html.

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: