Closed Bug 1314080 Opened 8 years ago Closed 8 years ago

Provide a ChromeOnly API for Range to deliver text content correlated to getClientRects

Categories

(Core :: Layout, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: bradwerth, Assigned: bradwerth)

References

Details

Attachments

(5 files)

Bug 1314080 Part 1: Rename CollectClientRects method and add a new parameter for collecting text.
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1314080 Part 2: Expand GetPartialTextRect with a new text collecting parameter.
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1314080 Part 3: Add a new ChromeOnly DOM method and wire it to CollectClientRectsAndText.
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1314080 Part 4: Implement the new text retrieval behavior.
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1314080 Part 5: Adds a mochitest to exercise getClientRectsAndTexts.
58 bytes, text/x-review-board-request
smaug
: review+
Details 
Browser team needs a way to correlate text to the rects returned by Range.getClientRects().
Please note that this API might be a good candidate for a Web Standards track in the WhatWG. Daniel would be able to tell, for sure. I think :)
The changes here seem to be almost entirely in /dom, so this really needs a DOM peer (or at least someone more familiar with nsRange than I am) to review the patches. (Plus, the WebIDL changes definitely need DOM peer review.)

Perhaps smaug could review this? He's reviewed some other changes in nsRange.cpp recently-ish (bug 1211894, bug 1290937).

(In reply to Mike de Boer [:mikedeboer] from comment #6)
> Please note that this API might be a good candidate for a Web Standards
> track in the WhatWG. Daniel would be able to tell, for sure. I think :)

(I'll defer to whoever ends up reviewing this.  I'm not super-familiar with DOM APIs, nor do I follow WHATWG stuff super-closely -- I'm more in the css world.)
Attachment #8806067 - Flags: review?(dholbert) → review?(bugs)
Attachment #8806068 - Flags: review?(dholbert) → review?(bugs)
Attachment #8806069 - Flags: review?(dholbert) → review?(bugs)
Attachment #8806070 - Flags: review?(dholbert) → review?(bugs)
Attachment #8806071 - Flags: review?(dholbert) → review?(bugs)
(In reply to Mike de Boer [:mikedeboer] from comment #6)
> Please note that this API might be a good candidate for a Web Standards
> track in the WhatWG. Daniel would be able to tell, for sure. I think :)

This kind of stuff is defined in CSSOM-View
https://drafts.csswg.org/cssom-view/#extensions-to-the-range-interface
Comment on attachment 8806067 [details]
Bug 1314080 Part 1: Rename CollectClientRects method and add a new parameter for collecting text.

https://reviewboard.mozilla.org/r/89602/#review89758

::: dom/base/nsRange.h:267
(Diff revision 1)
>     */
>    static bool IsNodeSelected(nsINode* aNode, uint32_t aStartOffset,
>                               uint32_t aEndOffset);
>  
> -  static void CollectClientRects(nsLayoutUtils::RectCallback* aCollector,
> +  static void CollectClientRectsAndText(nsLayoutUtils::RectCallback* aCollector,
> +                                        mozilla::dom::DOMStringList* aText,

I would have probably used sequence<DOMString> in the webidl and not DOMStringList, but doesn't matter  too much for ChromeOnly stuff.


Please document that aText can be null. Hmm, and rename the argument. aTextList ?
Attachment #8806067 - Flags: review?(bugs) → review+
Comment on attachment 8806068 [details]
Bug 1314080 Part 2: Expand GetPartialTextRect with a new text collecting parameter.

https://reviewboard.mozilla.org/r/89604/#review89762

::: dom/base/nsRange.cpp:2901
(Diff revision 1)
>    }
>    return nullptr;
>  }
>  
>  static nsresult GetPartialTextRect(nsLayoutUtils::RectCallback* aCallback,
> +                                   mozilla::dom::DOMStringList* aText,

aTextList or some such
Attachment #8806068 - Flags: review?(bugs) → review+
Comment on attachment 8806069 [details]
Bug 1314080 Part 3: Add a new ChromeOnly DOM method and wire it to CollectClientRectsAndText.

https://reviewboard.mozilla.org/r/89606/#review89776

::: dom/base/nsRange.cpp:3076
(Diff revision 1)
> +  if (!mStartParent) {
> +    return;
> +  }
> +
> +  aResult.mRectList.Construct();
> +  aResult.mRectList.Value() = new DOMRectList(static_cast<nsIDOMRange*>(this));

Can't you just pass the DOMRectList as a param to Construct

::: dom/base/nsRange.cpp:3079
(Diff revision 1)
> +
> +  aResult.mRectList.Construct();
> +  aResult.mRectList.Value() = new DOMRectList(static_cast<nsIDOMRange*>(this));
> +
> +  aResult.mTextList.Construct();
> +  aResult.mTextList.Value() = new DOMStringList();

Same here.
Attachment #8806069 - Flags: review?(bugs) → review+
Comment on attachment 8806070 [details]
Bug 1314080 Part 4: Implement the new text retrieval behavior.

https://reviewboard.mozilla.org/r/89608/#review89778
Attachment #8806070 - Flags: review?(bugs) → review+
Comment on attachment 8806071 [details]
Bug 1314080 Part 5: Adds a mochitest to exercise getClientRectsAndTexts.

https://reviewboard.mozilla.org/r/89610/#review89784

Some more tests added, r+

::: dom/base/test/chrome/test_range_getClientRectsAndTexts.html:43
(Diff revision 1)
> +  SimpleTest.finish();
> +}
> +</script>
> +</head>
> +<body onLoad="runTests()">
> +<div id="main" style="width: 150px; border: 1px solid black">Here are a lot of words Please use find to highlight some words that wrap across a line boundary and see what happens.</div>

Please add also some tests where element contains not only text but also other elements inside it.
Attachment #8806071 - Flags: review?(bugs) → review+
Comment on attachment 8806069 [details]
Bug 1314080 Part 3: Add a new ChromeOnly DOM method and wire it to CollectClientRectsAndText.

https://reviewboard.mozilla.org/r/89606/#review89806

::: dom/base/nsRange.cpp:3076
(Diff revision 1)
> +  if (!mStartParent) {
> +    return;
> +  }
> +
> +  aResult.mRectList.Construct();
> +  aResult.mRectList.Value() = new DOMRectList(static_cast<nsIDOMRange*>(this));

I haven't been able to get that to work. When I attempt something like...

aResult.mRectList.Construct(new DOMRectList(static_cast<nsIDOMRange*>(this)));

...I get a bunch of non-matching constructor errors. I can't figure out the templated emplace semantics for an Optional<OwningNonNull<T>>. I did an end-run around this by modifying the webidl and making the elements of the dictionary "required".
Comment on attachment 8806071 [details]
Bug 1314080 Part 5: Adds a mochitest to exercise getClientRectsAndTexts.

https://reviewboard.mozilla.org/r/89610/#review89784

> Please add also some tests where element contains not only text but also other elements inside it.

As long as you meant for it to include other text-containing elements, then the test now does what you ask. I will make further modification to the test to add some inline non-text-containing elements (img and such).

... and I have now done so. Behaves as expected across a non-text-containing inline element.
Comment on attachment 8806071 [details]
Bug 1314080 Part 5: Adds a mochitest to exercise getClientRectsAndTexts.

https://reviewboard.mozilla.org/r/89610/#review90180

::: dom/base/test/chrome/test_range_getClientRectsAndTexts.html:57
(Diff revision 4)
> +
> +<div id="embed.a">Line with <span>simple nested</span> <span id="embed.b">text</span></div>
> +
> +<div id="deep.a">Line with <span>complex, <span>more <span>deeply <span>nested</span></span></span></span> <span id="deep.b">text</span></div>
> +
> +<div id="image.a">Line with inline <img src="test_range_getClientRectsAndTexts.png" width="10" height="10"/> <span id="image.b">image</span></div>

Optional note: if you like, you can avoid the overhead of adding a this special single-use test-image-file.  You can instead simply use a data-URI-encoding of an image here.

See https://dxr.mozilla.org/mozilla-central/source/toolkit/content/tests/widgets/test_mousecapture_area.html for an example of this.

(If you want to do this, can use http://software.hixie.ch/utilities/cgi/data/data to create your own, or [better] just copypaste the one from that existing testcase.)
Comment on attachment 8806071 [details]
Bug 1314080 Part 5: Adds a mochitest to exercise getClientRectsAndTexts.

https://reviewboard.mozilla.org/r/89610/#review90180

> Optional note: if you like, you can avoid the overhead of adding a this special single-use test-image-file.  You can instead simply use a data-URI-encoding of an image here.
> 
> See https://dxr.mozilla.org/mozilla-central/source/toolkit/content/tests/widgets/test_mousecapture_area.html for an example of this.
> 
> (If you want to do this, can use http://software.hixie.ch/utilities/cgi/data/data to create your own, or [better] just copypaste the one from that existing testcase.)

Good idea, thank you for the guidance. I've made that change also.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c63b7e3446b5
Part 1: Rename CollectClientRects method and add a new parameter for collecting text. r=smaug
https://hg.mozilla.org/integration/autoland/rev/2eac07d441aa
Part 2: Expand GetPartialTextRect with a new text collecting parameter. r=smaug
https://hg.mozilla.org/integration/autoland/rev/b60df721f0e2
Part 3: Add a new ChromeOnly DOM method and wire it to CollectClientRectsAndText. r=smaug
https://hg.mozilla.org/integration/autoland/rev/4231c5d216b0
Part 4: Implement the new text retrieval behavior. r=smaug
https://hg.mozilla.org/integration/autoland/rev/3133acf27a5d
Part 5: Adds a mochitest to exercise getClientRectsAndTexts. r=smaug
Keywords: checkin-needed
I still thing, even though I'm biased, that this API is very well suited to becoming a web standard. It's the missing piece, I think, in making the Selection API truly bi-directional; selection mutators (scripts) don't need this because they add ranges to specific texts, thus rarely need to back-reference them. Selection accessors, on the other hand, do not yet have a way to get by the same information as the mutator has/ had, even though they're perfectly capable to access everything else and even alter the state the mutator initiated.

Flagging Anne, what do you think?
Flags: needinfo?(annevk)
Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1282752#c22 for a detailed explanation of how this API came to be.
Makes sense to me. Probably the best place to request this would be at: https://github.com/w3c/csswg-drafts/issues/new?title=[cssom-view] If that does not get you anywhere let me know.
Flags: needinfo?(annevk)
(In reply to Anne (:annevk) from comment #34)
> Makes sense to me. Probably the best place to request this would be at:
> https://github.com/w3c/csswg-drafts/issues/new?title=[cssom-view] If that
> does not get you anywhere let me know.

Brad, would you be interested in this kind of thing - to get your newly implemented Range extension be part of the spec and exposed to the Web - or rather not your cup of tea?
Flags: needinfo?(bwerth)
(In reply to Mike de Boer [:mikedeboer] from comment #35)
> Brad, would you be interested in this kind of thing - to get your newly
> implemented Range extension be part of the spec and exposed to the Web - or
> rather not your cup of tea?

Thank you for offering, but I suspect I would not be able to articulate as clearly as you have (e.g. in comment 32) the value of this API. If there are questions down the road, you're going to be better suited for defending it. I'm the person to bring in when and if changes are needed to make the API more acceptable to the standards body.
Flags: needinfo?(bwerth)
You're not going to get much love for anything involving DOMStringList. Why did we not use a sequence<DOMString>?
(In reply to :Ms2ger (⌚ UTC+1/+2) from comment #37)
> You're not going to get much love for anything involving DOMStringList. Why
> did we not use a sequence<DOMString>?

I found it easier to implement as a DOMStringList, and didn't know that it was bad practice. If it is problematic for the future, open a bug to switch it over and I'll take it on.
It looks like we return the DOM text in this API.  I wonder if not nsTextFrame::GetRenderedText
would be more appropriate?  The client rects corresponds to what's actually rendered so it
seems more consistent if the returned text is the rendered text.  That might be more useful
to web developers too.
Flags: needinfo?(bwerth)
Blocks: 1343695
(In reply to Mats Palmgren (:mats) from comment #39)
> It looks like we return the DOM text in this API.  I wonder if not
> nsTextFrame::GetRenderedText
> would be more appropriate?  The client rects corresponds to what's actually
> rendered so it
> seems more consistent if the returned text is the rendered text.  That might
> be more useful
> to web developers too.

Bug 1343695 opened to address this.
Flags: needinfo?(bwerth)
Blocks: 1343978
(In reply to :Ms2ger (⌚ UTC+1/+2) from comment #37)
> You're not going to get much love for anything involving DOMStringList. Why
> did we not use a sequence<DOMString>?

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

Attachment

General

Creator:
Created:
Updated:
Size: