1338898 - selectionText has 150 characters at most (browser.contextMenus.OnClickData)

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Geoffrey De Belie (Smile4ever)]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170203110047

Steps to reproduce:

Inside a WebExtension's background script:

browser.contextMenus.create({
	id: "translatenow-translate",
	title: "Translate Now",
	contexts: ["selection"]
}, onCreated);

browser.contextMenus.onClicked.addListener(listener);

function listener(info,tab){
	console.log(info.selectionText.length);
}


Actual results:

Selection text is never longer than 150 characters.


Expected results:

There shouldn't be a limit of 150 characters on the selectionText, but instead Firefox should return the whole selectionText. Chrome has no 150 character limit as far as I know.

Comment 1

[erosman]

I can also confirm that on FF53 making a selection longer than 150 characters will result in cropping.
console.log(info.selectionText.length) // result 150

Comment 2

[Markus Jaritz [:designakt] (UX)]

Do we know why we currently have this limit?
From what I see here it seams rather arbitrary with no reason or warning given.

Comment 3

[Andy McKay]

I think we just didn't update the whiteboard, so doing that now.

Comment 4

[Shane Caraveo (:mixedpuppy)]

I'm inclined to dup this to bug 1325814.  Having the selector should allow an addon to retrieve large text chunks.  The existing 150 char limit should be fine otherwise and avoid overhead when unnecessary.

Comment 5

[erosman]

Pleas note that the limit, forces developers to inject script into content to grab the selection text. Besides the fact that injecting script is prevented by Firefox on certain pages, it is a hack just to get the info.selectionText

Why was the limit imposed by Firefox and what is the benefit?

Comment 6

[Geoffrey De Belie (Smile4ever)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> I'm inclined to dup this to bug 1325814.  Having the selector should allow
> an addon to retrieve large text chunks.  The existing 150 char limit should
> be fine otherwise and avoid overhead when unnecessary.

It doesn't solve the case where you select text on priviledged websites or websites that use complex iframe/frame handling if the only thing you're doing is passing a selector.

Getting selection text from iframes and frames is hard because of the same-origin policy. Some websites extensively use iframes from third parties, like comment sections. You will want to support that as well without hitting a cross-domain policy violation. However, a selector is useless when you're dealing with iframes (there is no way to get the element).

You'll also need a content script to support this use case, so you're injecting JavaScript into a page to get the selection. Isn't that a bit ridiculous if all you're doing is getting the selection text?

(In reply to erosman from comment #5)
> Pleas note that the limit, forces developers to inject script into content
> to grab the selection text. Besides the fact that injecting script is
> prevented by Firefox on certain pages, it is a hack just to get the
> info.selectionText
> 
> Why was the limit imposed by Firefox and what is the benefit?

I wish I could upvote this. I would also like to know why there is a limit of 150 characters. It may have nothing to do with performance, after all.

Comment 7

[Shane Caraveo (:mixedpuppy)]

The 150 char limit was originally added in bug 221361 to deal with performance issues.  In today's world, sending arbitrary sized data across processes for any context menu, when its unlikely the majority need the data, is not the solution either.   

I agree you need *a way* to get at the selection, and it is preferable that you don't need to resort to a content script if this would be the only reason.  I think an addon should specifically ask for it if it needs the data.  The question is the best approach for that.  I still need to dig around more.  Input is welcome.

Smile4Ever: I'm not certain that selected text should be available if an addon does not otherwise have access to the site.

Comment 8

[erosman]

That makes sense and I agree since I also always consider optimizing performance ;)
In that case, what about a new flag eg "fullText: true" (defaults to false) to enable grabbing the full text?

Comment 9

[Geoffrey De Belie (Smile4ever)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> The 150 char limit was originally added in bug 221361 to deal with
> performance issues.

Weird that it was never documented.

> In today's world, sending arbitrary sized data across
> processes for any context menu, when its unlikely the majority need the
> data, is not the solution either.   

Indeed.

> I agree you need *a way* to get at the selection, and it is preferable that
> you don't need to resort to a content script if this would be the only
> reason.  I think an addon should specifically ask for it if it needs the
> data.  The question is the best approach for that.  I still need to dig
> around more.  Input is welcome.

Glad you say a content script is not the ideal solution here. Also, there are many options to implement "a way", see for example my suggestion at the bottom of this comment. A selection API for WebExtensions might be an option as well (set/get/clear selection text).

> Smile4Ever: I'm not certain that selected text should be available if an
> addon does not otherwise have access to the site.

The addon has a context menu item on addons.mozilla.org (just like every other page) so I should do something sensible there. My addon was even praised for supporting translations on addons.mozilla.org in a review :)

(In reply to erosman from comment #8)
> That makes sense and I agree since I also always consider optimizing
> performance ;)
> In that case, what about a new flag eg "fullText: true" (defaults to false)
> to enable grabbing the full text?

Or this, which makes it easier to add additional options?

browser.contextMenus.create({
	id: "translatenow-translate",
	title: "Translate Now",
	contexts: ["selection"],
	options: ["fulltext"]
}, onCreated);

browser.contextMenus.onClicked.addListener(listener);

function listener(info,tab){
	console.log(info.selectionText.length);
}

Comment 10

[erosman]

I have just noticed.... bug 221361 relates to 14 years ago O_O
 
They were talking about low-spec computers and old FF & JS engines.

Both computers and Firefox core and its JS engine improved so much that they are no longer comparable with the time that the bug was filled for XUL based operation.

Someone has to do a performance test before summarily dismissing the issue.

AFA I know, Chrome doesn't have to truncate selectionText. Is that to admit that Firefox code isn't as efficient?

Comment 11

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1338898 fix 150 char limit for context menu text selection,

Review commit: https://reviewboard.mozilla.org/r/138178/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138178/

Comment 12

[Andrew Swan [:aswan]]

Comment on attachment 8866572 [details]
Bug 1338898 fix 150 char limit for context menu text selection,

https://reviewboard.mozilla.org/r/138178/#review141422

I'm not sure I'm best qualified to review this.  The safety of this method depends on a bunch of assumptions about simultaneous actions and how quickly state in the content process can change.  I simply don't know enough to evaluate those assumptions.  Kris has his hands full right now, perhaps a dedicated front-end developers like Florian or Gijs?

::: browser/components/extensions/ext-contextMenus.js:265
(Diff revision 1)
> +      // If we are at that limit, it's a good assumption there is more, we want
> +      // to provide it all to extensions.
> +      if (info.selectionText && info.selectionText.length == 150) {
> +        let browser = contextData.tab.linkedBrowser;
> +        let mm = browser.messageManager;
> +        mm.addMessageListener("Browser:GetSelectionDone", function getSelection(message) {

Without knowing the bigger picture here, this seems questionable for a couple of reasons
1. Can there be two copies of this code running simultaneously (i.e., two different extensions or clicks in two different tabs or windows)?  If so, it seems like there's a danger of one instance receiving the response meant for a different instance
2. Since this is asynchronous, can the focus/selection/etc between the time this request gets sent to the content process and the content process gets around to handing it?

Comment 13

[Geoffrey De Belie (Smile4ever)]

> Without knowing the bigger picture here, this seems questionable for a
> couple of reasons
> 1. Can there be two copies of this code running simultaneously (i.e., two
> different extensions or clicks in two different tabs or windows)?  If so, it
> seems like there's a danger of one instance receiving the response meant for
> a different instance

You could check the first 150 characters of the full selection text and compare that to the selection text you already have to avoid this problem.

Comment 14

[Tomislav Jovanovic :zombie]

Comment on attachment 8866572 [details]
Bug 1338898 fix 150 char limit for context menu text selection,

https://reviewboard.mozilla.org/r/138178/#review141422

> Without knowing the bigger picture here, this seems questionable for a couple of reasons
> 1. Can there be two copies of this code running simultaneously (i.e., two different extensions or clicks in two different tabs or windows)?  If so, it seems like there's a danger of one instance receiving the response meant for a different instance
> 2. Since this is asynchronous, can the focus/selection/etc between the time this request gets sent to the content process and the content process gets around to handing it?

1) There should never be two copies of this code, as the module is loaded only once in the parent process, and most of the code is "global", except for the ExtensionAPI at the bottom, which is per extension/context.  Further, it would be hard to click two context menu items in two different tabs so quickly ;), but even then, this is sending to, and listening for response from a message manager tied to each tab, so they shouldn't collide.

2) There's an even bigger issue here, since the user is involved between opening the context menu and clicking the menu item, the selection can already change during that time.  And even the action of clicking itself already involves a menu item in the parent, sending the message to content process, that gathers the information and returns it via another message.

This doesn't change that process all that much (just an extra round trip), so I'm wondering if it's even significantly better than just sending the whole selection (but only if we actually have an extensions with "selection" context active.

Comment 15

[erosman]

> if we actually have an extensions with "selection" context active
That is a good optimizer to avoid bouncing unnecessary data around

Comment 16

[Shane Caraveo (:mixedpuppy)]

(In reply to Tomislav Jovanovic :zombie from comment #14)
> Comment on attachment 8866572 [details]
> Bug 1338898 fix 150 char limit for context menu text selection,
> 
> https://reviewboard.mozilla.org/r/138178/#review141422
> 
> > Without knowing the bigger picture here, this seems questionable for a couple of reasons
> > 1. Can there be two copies of this code running simultaneously (i.e., two different extensions or clicks in two different tabs or windows)?  If so, it seems like there's a danger of one instance receiving the response meant for a different instance
> > 2. Since this is asynchronous, can the focus/selection/etc between the time this request gets sent to the content process and the content process gets around to handing it?

I don't see any issues in either case, much of the reason covered by zombie.  We're already in full async mode even before the context menu is shown, this all should happen fast enough that the user would never notice.  There could be a race with the selection being changed or removed between the context menu being shown and the user clicking on a context menu item.  I'm not convinced that's a real problem, though the code could fall back if no selection is returned.

> This doesn't change that process all that much (just an extra round trip),
> so I'm wondering if it's even significantly better than just sending the
> whole selection (but only if we actually have an extensions with "selection"
> context active.

The benefit of doing this in the context menu click handler is that we only retrieve large data when an addon context menu looking for selection is actually clicked on.  Placing it up front means that every context click would require retrieval of the full selection [if limiting to when an addon is installed, it still affects everything once that happens].  Without doing a full perf/memory test it's hard to know if it's really an issue and I think that's a good question to ask.  If it's not an issue then we'd just remove the 150 char limit and be done.  

We could also increase that limit, I really don't think a couple K bytes would be a big deal, and that would reduce this code path to a very rare use case.  

I'll dig up a couple other opinions.

Comment 17

[Tomislav Jovanovic :zombie]

(In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> The benefit of doing this in the context menu click handler is that we only
> retrieve large data when an addon context menu looking for selection is
> actually clicked on.  Placing it up front means that every context click
> would require retrieval of the full selection 

Ah right, I got things slightly mixed up, the messaging/content inspection is currently only done upon opening the context menu, and not when the user clicks the menu item -- your approach makes more sense now (and bumping the default limit to a few K bytes shouldn't really be controversial).

Comment 18

[Shane Caraveo (:mixedpuppy)]

(In reply to Tomislav Jovanovic :zombie from comment #17)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> > The benefit of doing this in the context menu click handler is that we only
> > retrieve large data when an addon context menu looking for selection is
> > actually clicked on.  Placing it up front means that every context click
> > would require retrieval of the full selection 
> 
> Ah right, I got things slightly mixed up, the messaging/content inspection
> is currently only done upon opening the context menu, and not when the user
> clicks the menu item -- 

To be clear, with this patch, the full text retrieval happens when the user clicks on the extensions menuitem, not when the context menu is opened.

Comment 19

[Andrew Swan [:aswan]]

Comment on attachment 8866572 [details]
Bug 1338898 fix 150 char limit for context menu text selection,

https://reviewboard.mozilla.org/r/138178/#review143588

Clearing r? this should go to somebody with more expertise in the issues raised previously.

Comment 20

[Matthew Wein [:mattw]]

Comment on attachment 8866572 [details]
Bug 1338898 fix 150 char limit for context menu text selection,

https://reviewboard.mozilla.org/r/138178/#review143642

The 150 character limit was introduced 14 years ago, so I'd really like to know first if this limit is needed anymore before we go this route.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Mossop: I've done some rudimentary timing using a 3.5mb selection (specifically removing the part of code in browser-content.js that actually considers that limit).  It doesn't seem to have any greater overhead than 150 characters.  

Do you have any thoughts around this?

The current patch I did does a full selection retrieval when the extension context menuitem is clicked on, so a second async round trip.  Other direction would be removing the limit.

One other consideration: The current code removes redundant whitespace and newlines.  Should a full-text selection include those?

Comment 22

[Dave Townsend [:mossop]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> Mossop: I've done some rudimentary timing using a 3.5mb selection
> (specifically removing the part of code in browser-content.js that actually
> considers that limit).  It doesn't seem to have any greater overhead than
> 150 characters.  
> 
> Do you have any thoughts around this?

What hardware did you test that on?

> The current patch I did does a full selection retrieval when the extension
> context menuitem is clicked on, so a second async round trip.  Other
> direction would be removing the limit.

That seems like a good middle ground here if the longer selection is a problem.

Comment 23

[erosman]

> One other consideration: The current code removes redundant whitespace and newlines.  Should a full-text selection include those?

Please keep the selection verbatim and without any change. 

I specifically have to tabs.executeScript ... window.getSelection().toString(); because "info.selectionText doesn't retain line-breaks"

Comment 24

[Shane Caraveo (:mixedpuppy)]

(In reply to Dave Townsend [:mossop] from comment #22)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> > Mossop: I've done some rudimentary timing using a 3.5mb selection
> > (specifically removing the part of code in browser-content.js that actually
> > considers that limit).  It doesn't seem to have any greater overhead than
> > 150 characters.  
> > 
> > Do you have any thoughts around this?
> 
> What hardware did you test that on?

A fairly modern mac air.  I had to run the code a few hundred thousand times to get meaningful differences, but it was just a quick test in scratchpad.  My assumption is that the differences really depended on what else was happening.

> > The current patch I did does a full selection retrieval when the extension
> > context menuitem is clicked on, so a second async round trip.  Other
> > direction would be removing the limit.
> 
> That seems like a good middle ground here if the longer selection is a
> problem.

I'm inclined to keep it going in this direction (second async round trip) to get the selection rather than retrieving it for the larger use case that wont need it.  What do you think?

Comment 25

[Dave Townsend [:mossop]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #24)
> (In reply to Dave Townsend [:mossop] from comment #22)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> > > The current patch I did does a full selection retrieval when the extension
> > > context menuitem is clicked on, so a second async round trip.  Other
> > > direction would be removing the limit.
> > 
> > That seems like a good middle ground here if the longer selection is a
> > problem.
> 
> I'm inclined to keep it going in this direction (second async round trip) to
> get the selection rather than retrieving it for the larger use case that
> wont need it.  What do you think?

Sounds good to me

Comment 26

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8866572 [details]
Bug 1338898 fix 150 char limit for context menu text selection,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138178/diff/1-2/

Comment 27

[Shane Caraveo (:mixedpuppy)]

zombie, can you review for webext, Gijs for general approach.

Comment 28

[:Gijs (out for now; he/him)]

Comment on attachment 8866572 [details]
Bug 1338898 fix 150 char limit for context menu text selection,

https://reviewboard.mozilla.org/r/138178/#review149174

I have some suggestions, but I think ultimately Bill or maybe :mrbkap or :Mossop would be a better reviewer for approach here. The 150ch limit is weird. What does our copy/paste context menu code do? Pretty sure that works for strings > 150ch. Why doesn't that run into this issue?

::: browser/components/extensions/ext-contextMenus.js:269
(Diff revision 2)
>          actionFor(item.extension).triggerAction(win);
>        }
> -
> +      // BrowserUtils.getSelectionDetails limits text selection to 150 chars.
> +      // If we are at that limit, it's a good assumption there is more, we want
> +      // to provide it all to extensions.
> +      if (info.selectionText && info.selectionText.length == 150) {

It's sad that you need to keep this magic number in two places. It seems like it might make sense to just set a bool on the data it returns as to whether the data was cut off - that'd be neater than hardcoding the limit in two places, and also comes into play for the following...

It also seems (from a very quick look, so I could be wrong) like getSelectionDetails does some editing on the selection in terms of whitespace etc. Do we need to match that?

Then, I believe there's an IPC message size limit. Do we care about hitting that?

Finally, can we just use this API instead of getSelection? It feels odd to need 2 messages here...

Maybe :billm would be a better reviewer here.

::: toolkit/content/browser-content.js:1080
(Diff revision 2)
> +  let selection = focusedWindow.getSelection();
> +  let selectionStr = selection.toString();

Nit: collapse to:


```js
let selectionStr = focusedWindow.getSelection().toString();
```

Comment 29

[Tomislav Jovanovic :zombie]

Comment on attachment 8866572 [details]
Bug 1338898 fix 150 char limit for context menu text selection,

https://reviewboard.mozilla.org/r/138178/#review149504

This looks like a good approach, but I think we should go even further and only use this method to get the selected text, even when it's not truncated.

It looks that the current behavior of limiting to 150 characters and stripping whitespace (from bug 221361) was an optimization for the creations of the context menu.  The actual copy/paste operation doesn't seem to use any of this, and only grabs the selection when the user actually clicks `Copy` (I tested with a `setInterval` that changes the selection every second).

Therefore IMO we should change our behavior to align it with what we do for copy/paste, meaning: 1) never strip whitespace, and 2) always get the selection after the user selects the menu item.

::: browser/components/extensions/test/browser/browser_ext_contextMenus.js:282
(Diff revision 2)
>    checkClickInfo(result);
>    result = await extension.awaitMessage("browser.contextMenus.onClicked");
>    checkClickInfo(result);
>  
> +  // Select a lot of text
> +  await ContentTask.spawn(gBrowser.selectedBrowser, { }, function* (arg) {

nit: this doesn't need to be a generator, the argument is unused, and ditto below.

::: browser/components/extensions/test/browser/browser_ext_contextMenus.js:298
(Diff revision 2)
> +  // Bring up context menu again
> +  extensionMenuRoot = await openExtensionContextMenu("#longtext");
> +
> +  // Check some menu items
> +  items = extensionMenuRoot.getElementsByAttribute("label", "selection is: 'Sed ut perspiciatis unde omnis iste natus err...'");
> +  is(items.length, 1, `contextMenu item for selection was found (context=selection) - ${items[0].label}`);

`items[0].label` will throw when no items are found, and logging it doesn't seem useful when the test passes.

::: browser/components/extensions/test/browser/browser_ext_contextMenus.js:310
(Diff revision 2)
> +
> +  result = await extension.awaitMessage("onclick");
> +  checkClickInfo(result);
> +  result = await extension.awaitMessage("browser.contextMenus.onClicked");
> +  checkClickInfo(result);
> +  is(result.info.selectionText.length, 865, "long text selection worked");

How about a test that `selectionText.endsWith()` the correct string at least?

::: browser/components/extensions/test/browser/browser_ext_contextMenus.js:317
(Diff revision 2)
> +    let selection = content.getSelection();
> +    selection.removeAllRanges();

Is this still needed here?

::: browser/components/extensions/test/browser/browser_ext_contextMenus.js:319
(Diff revision 2)
> +  // the Browser:GetSelection handler.
> +  await ContentTask.spawn(gBrowser.selectedBrowser, { }, function* (arg) {
> +    let doc = content.document;
> +    let selection = content.getSelection();
> +    selection.removeAllRanges();
> +    let textNode = doc.getElementById("editabletext");

nit: this is not a `textNode` here

::: browser/components/extensions/test/browser/browser_ext_contextMenus.js:329
(Diff revision 2)
> +  expectedClickInfo = {
> +    menuItemId: "ext-selection",
> +    pageUrl: PAGE,
> +  };

This is unused.

::: browser/components/extensions/test/browser/context.html:16
(Diff revision 2)
>      <a href="some-link" id="link1">Some link</a>
>    </p>
>  
>    <p>
>      <a href="image-around-some-link">
> -      <img src="ctxmenu-image.png" id="img-wrapped-in-link">
> +      <img src="ctxmenu-image.png" id="img-wrapped-in-link" />

nit: this is not needed

::: browser/components/extensions/test/browser/context.html:21
(Diff revision 2)
> -    <input type="text" id="edit-me"><br>
> -    <input type="password" id="password">
> +    <input type="text" id="edit-me"></input><br>
> +    <input type="password" id="password"></input>

nit: and this is actually against the spec: http://w3c.github.io/html-reference/syntax.html#void-element_xref3

::: toolkit/content/browser-content.js:1076
(Diff revision 2)
>    }
>  });
>  
> +// Similar to BrowserUtils.getSelectionDetails, but returns the full text of
> +// the selection and doesn't bother with link detection, etc.
> +addMessageListener("Browser:GetSelection", function BrowserGetSelection() {

What's the reason for adding this here, and not in `extension-process-script.js`?  Did you try to find the code that handles copy/paste as Gijs mentioned, maybe we can reuse that instead?

::: toolkit/content/browser-content.js:1084
(Diff revision 2)
> +  focusedWindow = focusedWindow.value;
> +  let selection = focusedWindow.getSelection();
> +  let selectionStr = selection.toString();
> +
> +  // try getting a selected text in text input.
> +  if (!selectionStr && focusedElement instanceof Ci.nsIDOMNSEditableElement) {

Isn't `nsIDOMNSEditableElement` check equivalent to the `instanceof` checks below.

Also, I think `selectionStr` is always going to be falsy when `focusedElement` is an editable element, so this whole if statement might be redundant.

Comment 30

[Tomislav Jovanovic :zombie]

Comment on attachment 8866572 [details]
Bug 1338898 fix 150 char limit for context menu text selection,

https://reviewboard.mozilla.org/r/138178/#review149174

The 150 char limit is only for creating the context menu items, since some of them can reference the selected text: `Search Google for "selected text trunca..."`.

I couldn't easily find the exact code that does it, but actual copy/paste is likely totally independent from this, as it 1) doesn't trim any whitespace, and 2) only grabs the selection when the user actually clicks `Copy`.

Comment 31

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8866572 [details]
Bug 1338898 fix 150 char limit for context menu text selection,

https://reviewboard.mozilla.org/r/138178/#review149504

> This is unused.

It is used in checkClickInfo

> nit: this is not needed

It's used in a different test file.

> What's the reason for adding this here, and not in `extension-process-script.js`?  Did you try to find the code that handles copy/paste as Gijs mentioned, maybe we can reuse that instead?

IIUC cmd_copy is handled down in nsGlobalWindowCommands.cpp, and we only implement overrides for specific situations.  Reusing that code doesn't really make sense here.

This operation needs to happen in the browser content process, it doesn't make sense to place it into the extension process script.

Comment 32

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8866572 [details]
Bug 1338898 fix 150 char limit for context menu text selection,

https://reviewboard.mozilla.org/r/138178/#review149174

> It's sad that you need to keep this magic number in two places. It seems like it might make sense to just set a bool on the data it returns as to whether the data was cut off - that'd be neater than hardcoding the limit in two places, and also comes into play for the following...
> 
> It also seems (from a very quick look, so I could be wrong) like getSelectionDetails does some editing on the selection in terms of whitespace etc. Do we need to match that?
> 
> Then, I believe there's an IPC message size limit. Do we care about hitting that?
> 
> Finally, can we just use this API instead of getSelection? It feels odd to need 2 messages here...
> 
> Maybe :billm would be a better reviewer here.

I think we can just always get the data for addons.  The larger issue here is the IPC message size which is 64k.  That may be an issue that causes this to be rethought.  Not clear what you mean by "can we just use this API instead of getSelection".

Comment 33

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8866572 [details]
Bug 1338898 fix 150 char limit for context menu text selection,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138178/diff/2-3/

Comment 34

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8866572 [details]
Bug 1338898 fix 150 char limit for context menu text selection,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138178/diff/3-4/

Comment 35

[Matthew Wein [:mattw]]

Comment on attachment 8866572 [details]
Bug 1338898 fix 150 char limit for context menu text selection,

https://reviewboard.mozilla.org/r/138178/#review158618

::: browser/base/content/nsContextMenu.js:119
(Diff revision 4)
>          selectionText: this.isTextSelected ? this.selectionInfo.text : undefined,
> +        fullText: this.isTextSelected ? this.selectionInfo.fullText : undefined,

I think "selectionText" implies "fullText", so using both is confusing. I think we should rename the existing implementation of "selectionText" to "trimmedText", since that more accurately describes what it is, and then we can re-purpose "selectionText" to include the full, unmodified text.

Comment 36

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8866572 [details]
Bug 1338898 fix 150 char limit for context menu text selection,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138178/diff/4-5/

Comment 37

[:Gijs (out for now; he/him)]

Comment on attachment 8866572 [details]
Bug 1338898 fix 150 char limit for context menu text selection,

https://reviewboard.mozilla.org/r/138178/#review158650

::: browser/components/extensions/ext-contextMenus.js:269
(Diff revision 2)
>          actionFor(item.extension).triggerAction(win);
>        }
> -
> +      // BrowserUtils.getSelectionDetails limits text selection to 150 chars.
> +      // If we are at that limit, it's a good assumption there is more, we want
> +      // to provide it all to extensions.
> +      if (info.selectionText && info.selectionText.length == 150) {

WRT 'just use this API' - in this old version of the patch we basically get the selection text once, using the thing that got us the truncated 150char value, and then we fetch the selected text a second time from the content process with the "Browser:GetSelection" message. If what we're after is the selected text, then we could just use the latter API/messaging directly rather than bothering with the thing that got us 'info' here, is what I was trying to suggest.

Comment 38

[Matthew Wein [:mattw]]

Comment on attachment 8866572 [details]
Bug 1338898 fix 150 char limit for context menu text selection,

https://reviewboard.mozilla.org/r/138178/#review158728

LGTM!

Comment 39

[Andrew Swan [:aswan]]

Comment on attachment 8866572 [details]
Bug 1338898 fix 150 char limit for context menu text selection,

https://reviewboard.mozilla.org/r/138178/#review158852

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9f2cca17fa88
fix 150 char limit for context menu text selection, r=aswan,mattw

Comment 41

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9f2cca17fa88

Comment 42

[erosman]

I noticed selectionText length is truncated at 16,384 (16k). Is that by design and expected?

Comment 43

[:Gijs (out for now; he/him)]

(In reply to erosman from comment #42)
> I noticed selectionText length is truncated at 16,384 (16k). Is that by
> design and expected?

Yes:

https://hg.mozilla.org/integration/autoland/rev/9f2cca17fa88#l5.30