987040 - Implement mozbrowserSelection

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[StevenLee[:slee]]

Implement the api, https://wiki.mozilla.org/User:Timdream/mozbrowserSelection, needed for Gaia.

Comment 1

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: (WIP) add-browserapi

Implementing new browser api, right now text and boundingClientRect is valid when we receive mozbrowseselectionchange event. We might able to show "popup" menu in Gaia base on boundingClientRect now.

Comment 2

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: (WIP) add-browserapi v2

This is a simple prototype.

Comment 3

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: (WIP) copy and paste popup bubble

Popup window for gaia

Comment 4

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8401137 [details] [diff] [review]
(WIP) copy and paste popup bubble

Hi Alive,
Can you give some feedback for those WIP? Thanks.

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8401137 [details] [diff] [review]
(WIP) copy and paste popup bubble

Review of attachment 8401137 [details] [diff] [review]:
-----------------------------------------------------------------

Well done! Please have unit test if you want to finish it.

::: apps/system/js/app_window.js
@@ +559,5 @@
>    AppWindow.REGISTERED_EVENTS =
>      ['mozbrowserclose', 'mozbrowsererror', 'mozbrowservisibilitychange',
>       'mozbrowserloadend', 'mozbrowseractivitydone', 'mozbrowserloadstart',
>       'mozbrowsertitlechange', 'mozbrowserlocationchange',
> +     'mozbrowsericonchange', 'mozbrowserselectionchange',

You don't need this change. Handle it in text selection dialog.

@@ +754,5 @@
>        this.config.favicon = evt.detail;
>        this.publish('iconchange');
>      };
>  
> +  AppWindow.prototype._handle_mozbrowserselectionchange =

Remove this.

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8401135 [details] [diff] [review]
(WIP) add-browserapi v2

Review of attachment 8401135 [details] [diff] [review]:
-----------------------------------------------------------------

No idea for gecko change, clearing.

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

I proposed to use 'mozbrowsertextualmenu' because we don't want copy/paste popup if the app/page is using range select API to select something.

Comment 8

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: (WIP) copy and paste popup bubble v2

support system app using mozChromeEvent

Comment 9

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: (WIP) add-browserapi v3

support system app using mozChromeEvent

Comment 10

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: (WIP) add-browserapi v4

using nsClipboard api to store copy&paste data
remove some useless debug code

Comment 11

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: (WIP) add-browserapi v5

Comment 12

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: (WIP) copy and paste popup bubble v3

Comment 13

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: (WIP) add-browserapi v6

Resolved a bug which the element inside iframe of iframe cannot receive selection event. Removed some hacky code and code clean up.

Comment 14

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: (WIP) copy and paste popup bubble v4

rebase to latest revision

Comment 15

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: (WIP) copy and paste popup bubble v5

Add missing display handle for select all element.

Comment 16

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: (WIP) add-browserapi v7

Most of features are working now. Fabrice, could you give some feedback before I send review request? Thanks.

Comment 17

[[:fabrice] Fabrice Desré]

Comment on attachment 8425373 [details] [diff] [review]
(WIP) add-browserapi v7

Review of attachment 8425373 [details] [diff] [review]:
-----------------------------------------------------------------

I took a quick look, but I really need to know where TextualMenuClose is coming from..
Also, you want someone that knows the editor/selection code better than me to review. I would ask Ehsan.

::: b2g/chrome/content/shell.js
@@ +526,5 @@
>          this.notifyContentStart();
>         break;
>  
> +      case 'mozbrowsertextualmenu':
> +        shell.sendEvent(getContentWindow(), 'mozChromeEvent', {

Use shell.sendCustomEvent() instead, with an event name that is not mozChromeEvent.

::: dom/browser-element/BrowserElementChildPreload.js
@@ +69,5 @@
> + */
> +function ns() {
> +  const map = new WeakMap();
> +  return function namespace(target) {
> +    if (!target)        // If `target` is not an object return `target` itself.

here and everywhere else: use {} even with single line if's

@@ +81,5 @@
> +  };
> +};
> +
> +const selections = ns();
> +var cachedSelection = null;

here and everywhere else: s/var/let

@@ +145,5 @@
> +      sendAsyncMsg("textualmenu", detail);
> +    }
> +  },
> +
> +  notifySelectionChanged: function (document, selection, reason) {

nit: function(...)

@@ +323,5 @@
> +    addEventListener('TextualMenuClose',
> +                     this._textualMenuCloseHandler.bind(this),
> +                     /* useCapture = */ true,
> +                     /* wantsUntrusted = */ false);
> +

What is triggering this event? I could not find any reference to a TextualMenuClose in the tree.

Comment 18

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Fabrice Desré [:fabrice] from comment #17)
> Comment on attachment 8425373 [details] [diff] [review]
> (WIP) add-browserapi v7
> 
> Review of attachment 8425373 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I took a quick look, but I really need to know where TextualMenuClose is
> coming from..
> Also, you want someone that knows the editor/selection code better than me
> to review. I would ask Ehsan.
> 
> ::: b2g/chrome/content/shell.js
> @@ +526,5 @@
> >          this.notifyContentStart();
> >         break;
> >  
> > +      case 'mozbrowsertextualmenu':
> > +        shell.sendEvent(getContentWindow(), 'mozChromeEvent', {
> 
> Use shell.sendCustomEvent() instead, with an event name that is not
> mozChromeEvent.
I call defineAndExpose to define some function in detail object (please refer to BrowserElementParent.jsm changes). If I call sendCustomEvent() it will fail at Cu.cloneInto(). So I have to call sendEvent directly.
> 
> ::: dom/browser-element/BrowserElementChildPreload.js
> @@ +69,5 @@
> > + */
> > +function ns() {
> > +  const map = new WeakMap();
> > +  return function namespace(target) {
> > +    if (!target)        // If `target` is not an object return `target` itself.
> 
> here and everywhere else: use {} even with single line if's
> 
done
> @@ +81,5 @@
> > +  };
> > +};
> > +
> > +const selections = ns();
> > +var cachedSelection = null;
> 
> here and everywhere else: s/var/let
> 
done
> @@ +145,5 @@
> > +      sendAsyncMsg("textualmenu", detail);
> > +    }
> > +  },
> > +
> > +  notifySelectionChanged: function (document, selection, reason) {
> 
> nit: function(...)
> 
done
> @@ +323,5 @@
> > +    addEventListener('TextualMenuClose',
> > +                     this._textualMenuCloseHandler.bind(this),
> > +                     /* useCapture = */ true,
> > +                     /* wantsUntrusted = */ false);
> > +
> 
> What is triggering this event? I could not find any reference to a
> TextualMenuClose in the tree.
TextualMenuClose event is triggered by SelectionCaret. When selection caret is dragging or screen is scrolling, TextualMenuClose will fire. This code is in bug 987718 now, not landed yet.

Comment 19

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: (WIP) add-browserapi v8

Update to address comment

Hi Ehsan,
Can you give some feedback about editor/selection change in this patch? Thanks.

Comment 20

[(no longer active)]

Comment on attachment 8426025 [details] [diff] [review]
(WIP) add-browserapi v8

Review of attachment 8426025 [details] [diff] [review]:
-----------------------------------------------------------------

I have a hard time understanding what the API outlines here <https://wiki.mozilla.org/User:Timdream/mozbrowserSelection> is trying to achieve.  Not sure where the API was discussed, but the wiki page lacks a lot of details on what the API should do (for example it's not clear to me whether this is trying to handle input/textarea, contentEditable regions, or all web content, or a combination of the above.  Plus the patch doesn't even implement what that wiki page describes, so it's really hard to figure out what the intention is, but depending on what we want to achieve, the general approach of the patch may or may not be correct.  I'm going to provide some hints on the things that are obviously wrong in the patch but I need to know more about what we're trying to achieve here to be able to provide better feedback.

Note that I didn't look at the parts of the patch that I have not commented on very carefully.

::: dom/browser-element/BrowserElementChildPreload.js
@@ +96,5 @@
> +          domRange.startContainer.nodeType == Ci.nsIDOMNode.ELEMENT_NODE &&
> +          domRange.startContainer.hasChildNodes() &&
> +          domRange.startOffset < domRange.startContainer.childNodes.length &&
> +          domRange.startContainer.childNodes[domRange.startOffset].nodeType
> +            == Ci.nsIDOMNode.ELEMENT_NODE) {

Not sure what this check is trying to do.

@@ +102,5 @@
> +        boundingClientRect = element.getBoundingClientRect();
> +      }
> +
> +      let clipboardService = Cc["@mozilla.org/widget/clipboard;1"].
> +                             getService(Ci.nsIClipboard);

Question about the general approach.  Why don't we perform the clipboard operation in the child process?

@@ +123,5 @@
> +                     canPaste: clipboardHasData,
> +                     show: true,
> +                     zoomFactor: zoomFactor};
> +
> +      let elt = this.getFocusedElement();

It's not clear to me why we're tying things to the focused element.  That is often where the selection is but you could manually move both the focus and the selection in ways that gets them out of sync.  On desktop at least we always try to do copy/paste/etc based on the selection and not which element currently has the focus.

@@ +124,5 @@
> +                     show: true,
> +                     zoomFactor: zoomFactor};
> +
> +      let elt = this.getFocusedElement();
> +      if (elt && elt.editor) {

See my comment below about accessing .editor.

@@ +142,5 @@
> +        detail.right += nowRect.left;
> +        nowWindow = nowWindow.parent;
> +      }
> +
> +      cachedSelection = selection;

What guarantees that this cached selection is up to date by the time we try to use it?

@@ +143,5 @@
> +        nowWindow = nowWindow.parent;
> +      }
> +
> +      cachedSelection = selection;
> +      sendAsyncMsg("textualmenu", detail);

I assume this should be "contextualmenu"?

@@ +148,5 @@
> +    }
> +  },
> +
> +  notifySelectionChanged: function(document, selection, reason) {
> +    // We only care about mouse up here

Why?

@@ +167,5 @@
> +  },
> +
> +  onFocus: function() {
> +    let elt = selectionListener.getFocusedElement();
> +    if (!elt || !elt.editor) {

Not sure what kinds of elements you're expecting to see here, but I'm guessing this is accessing <http://mxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLInputElement.webidl?force=1#177>, so you seem to be only trying to handle input/textarea.  Is that correct?

I really don't think that's the right thing to do...

@@ +170,5 @@
> +    let elt = selectionListener.getFocusedElement();
> +    if (!elt || !elt.editor) {
> +      return;
> +    }
> +    let selection = elt.editor.selection;

So depending on what we want to do, this may be the wrong selection object.  We have one global selection object per document and also one selection object per input/textarea.  This code seems to be tailored to disregard the global selection object, and I think that's wrong.

@@ +176,5 @@
> +      return;
> +    }
> +    // Don't add the selection's listener more than once to the same window,
> +    // if the selection object is the same
> +    if (!("selection" in selections(elt) && selections(elt).selection === selection)) {

This is too much JS for my little brain.  :)  Not sure what this check tries to achieve, but it looks fishy.

@@ +182,5 @@
> +      // `nsISelectionPrivate` before working on it, in case is `null`.
> +      //
> +      // If it's `null` it's likely too early to add the listener, and we demand
> +      // that operation to `document-shown` - it can easily happens for frames
> +      if (selection instanceof Ci.nsISelectionPrivate) {

This will always be true.

@@ +183,5 @@
> +      //
> +      // If it's `null` it's likely too early to add the listener, and we demand
> +      // that operation to `document-shown` - it can easily happens for frames
> +      if (selection instanceof Ci.nsISelectionPrivate) {
> +        selection.addSelectionListener(selectionListener);

So this sets up one selection listener per input/textarea, but I don't see anywhere where we account for that in this patch, which seems to suggest that this patch should break if you have more than one of those elements on a page.

@@ +715,5 @@
> +  },
> +
> +  _contentLoadedHandler: function(e) {
> +    let doc = e.target;
> +    let selection = doc.getSelection();

Here you're looking at the global per-document selection object...

@@ +1181,5 @@
> +          cachedSelection.selectAllChildren(cachedSelection.focusNode.parentElement);
> +        } else if (cachedSelection.focusNode.nodeType == Ci.nsIDOMNode.ELEMENT_NODE) {
> +          cachedSelection.selectAllChildren(cachedSelection.focusNode);
> +        }
> +      }

Again, I'm not sure what this code is trying to do here, but this looks wrong to me.

@@ +1202,5 @@
> +      }
> +    }
> +  },
> +
> +  _copyToClipboard: function() {

Why are we inventing these new ways of copying/pasting/etc?  Why not use what we already support in Gecko?

@@ +1222,5 @@
> +            throw new Error("Couldn't set the clipboard due to an internal error " +
> +                            "(couldn't create a Transferable object).");
> +          }
> +          // Bug 769440: Starting with FF16, transferable have to be inited
> +          if ("init" in xferable) {

No need to test this.

Comment 21

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #20)
> Comment on attachment 8426025 [details] [diff] [review]
> (WIP) add-browserapi v8
> 
> Review of attachment 8426025 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have a hard time understanding what the API outlines here
> <https://wiki.mozilla.org/User:Timdream/mozbrowserSelection> is trying to
> achieve.  Not sure where the API was discussed, but the wiki page lacks a
> lot of details on what the API should do (for example it's not clear to me
> whether this is trying to handle input/textarea, contentEditable regions, or
> all web content, or a combination of the above.  Plus the patch doesn't even
> implement what that wiki page describes, so it's really hard to figure out
> what the intention is, but depending on what we want to achieve, the general
> approach of the patch may or may not be correct.  I'm going to provide some
> hints on the things that are obviously wrong in the patch but I need to know
> more about what we're trying to achieve here to be able to provide better
> feedback.
In 2.0, we'll handle element which is editable, that is, input/textarea and contenteditable elements.
> 
> Note that I didn't look at the parts of the patch that I have not commented
> on very carefully.
> 
> ::: dom/browser-element/BrowserElementChildPreload.js
> @@ +96,5 @@
> > +          domRange.startContainer.nodeType == Ci.nsIDOMNode.ELEMENT_NODE &&
> > +          domRange.startContainer.hasChildNodes() &&
> > +          domRange.startOffset < domRange.startContainer.childNodes.length &&
> > +          domRange.startContainer.childNodes[domRange.startOffset].nodeType
> > +            == Ci.nsIDOMNode.ELEMENT_NODE) {
> 
> Not sure what this check is trying to do.
http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsRange.cpp?from=nsRange.cpp#2824
According to the spec, we'll get empty client rect if range is collapsed and the node is not text node.
So in this case I need get correct client rect by manual.

> 
> @@ +102,5 @@
> > +        boundingClientRect = element.getBoundingClientRect();
> > +      }
> > +
> > +      let clipboardService = Cc["@mozilla.org/widget/clipboard;1"].
> > +                             getService(Ci.nsIClipboard);
> 
> Question about the general approach.  Why don't we perform the clipboard
> operation in the child process?
We actually perform the clipboard operation in the child process. The parent process is only responsible for showing utility bubble (such as copy, paste button).
> 
> @@ +123,5 @@
> > +                     canPaste: clipboardHasData,
> > +                     show: true,
> > +                     zoomFactor: zoomFactor};
> > +
> > +      let elt = this.getFocusedElement();
> 
> It's not clear to me why we're tying things to the focused element.  That is
> often where the selection is but you could manually move both the focus and
> the selection in ways that gets them out of sync.  On desktop at least we
> always try to do copy/paste/etc based on the selection and not which element
> currently has the focus.
I'll think about this problem.
> 
> @@ +124,5 @@
> > +                     show: true,
> > +                     zoomFactor: zoomFactor};
> > +
> > +      let elt = this.getFocusedElement();
> > +      if (elt && elt.editor) {
> 
> See my comment below about accessing .editor.
> 
> @@ +142,5 @@
> > +        detail.right += nowRect.left;
> > +        nowWindow = nowWindow.parent;
> > +      }
> > +
> > +      cachedSelection = selection;
> 
> What guarantees that this cached selection is up to date by the time we try
> to use it?
> 
> @@ +143,5 @@
> > +        nowWindow = nowWindow.parent;
> > +      }
> > +
> > +      cachedSelection = selection;
> > +      sendAsyncMsg("textualmenu", detail);
> 
> I assume this should be "contextualmenu"?
The wiki page says use "textualmenu"
> 
> @@ +148,5 @@
> > +    }
> > +  },
> > +
> > +  notifySelectionChanged: function(document, selection, reason) {
> > +    // We only care about mouse up here
> 
> Why?
Because we don't want to show utility bubble when user is changing the selection range. We only want to show bubble when user finish selection change. Mouse up is somehow a "finish" sign.
> 
> @@ +167,5 @@
> > +  },
> > +
> > +  onFocus: function() {
> > +    let elt = selectionListener.getFocusedElement();
> > +    if (!elt || !elt.editor) {
> 
> Not sure what kinds of elements you're expecting to see here, but I'm
> guessing this is accessing
> <http://mxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLInputElement.
> webidl?force=1#177>, so you seem to be only trying to handle input/textarea.
> Is that correct?
> 
> I really don't think that's the right thing to do...
Yap, this code handle only input/textarea because I cannot get all input/textarea in the beginning because I'll lose input/textarea which is dynamic created. So I listen to focus event and check if it has editor or not. If it has editor means it is input/textarea so I add a selection listener for it.

Contenteditable element is handled by global selection object. I add listener to global selection object when DOMContentLoaded event fired.
> 
> @@ +170,5 @@
> > +    let elt = selectionListener.getFocusedElement();
> > +    if (!elt || !elt.editor) {
> > +      return;
> > +    }
> > +    let selection = elt.editor.selection;
> 
> So depending on what we want to do, this may be the wrong selection object. 
> We have one global selection object per document and also one selection
> object per input/textarea.  This code seems to be tailored to disregard the
> global selection object, and I think that's wrong.
Mentioned above, onFocus only handled input/textarea.
> 
> @@ +176,5 @@
> > +      return;
> > +    }
> > +    // Don't add the selection's listener more than once to the same window,
> > +    // if the selection object is the same
> > +    if (!("selection" in selections(elt) && selections(elt).selection === selection)) {
> 
> This is too much JS for my little brain.  :)  Not sure what this check tries
> to achieve, but it looks fishy.
Just avoid to add duplicate selection listener to same element.
> 
> @@ +182,5 @@
> > +      // `nsISelectionPrivate` before working on it, in case is `null`.
> > +      //
> > +      // If it's `null` it's likely too early to add the listener, and we demand
> > +      // that operation to `document-shown` - it can easily happens for frames
> > +      if (selection instanceof Ci.nsISelectionPrivate) {
> 
> This will always be true.
> 
> @@ +183,5 @@
> > +      //
> > +      // If it's `null` it's likely too early to add the listener, and we demand
> > +      // that operation to `document-shown` - it can easily happens for frames
> > +      if (selection instanceof Ci.nsISelectionPrivate) {
> > +        selection.addSelectionListener(selectionListener);
> 
> So this sets up one selection listener per input/textarea, but I don't see
> anywhere where we account for that in this patch, which seems to suggest
> that this patch should break if you have more than one of those elements on
> a page.
> 
> @@ +715,5 @@
> > +  },
> > +
> > +  _contentLoadedHandler: function(e) {
> > +    let doc = e.target;
> > +    let selection = doc.getSelection();
> 
> Here you're looking at the global per-document selection object...
Yes this one add listener to global selection object.
> 
> @@ +1181,5 @@
> > +          cachedSelection.selectAllChildren(cachedSelection.focusNode.parentElement);
> > +        } else if (cachedSelection.focusNode.nodeType == Ci.nsIDOMNode.ELEMENT_NODE) {
> > +          cachedSelection.selectAllChildren(cachedSelection.focusNode);
> > +        }
> > +      }
> 
> Again, I'm not sure what this code is trying to do here, but this looks
> wrong to me.
This code is removed in next patch.
> 
> @@ +1202,5 @@
> > +      }
> > +    }
> > +  },
> > +
> > +  _copyToClipboard: function() {
> 
> Why are we inventing these new ways of copying/pasting/etc?  Why not use
> what we already support in Gecko?
I'll fix this in next patch.
> 
> @@ +1222,5 @@
> > +            throw new Error("Couldn't set the clipboard due to an internal error " +
> > +                            "(couldn't create a Transferable object).");
> > +          }
> > +          // Bug 769440: Starting with FF16, transferable have to be inited
> > +          if ("init" in xferable) {
> 
> No need to test this.

Comment 22

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: (WIP) add-browserapi v9

update to address comment

Comment 23

[(no longer active)]

(In reply to Morris Tseng [:mtseng] from comment #21)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #20)
> > Comment on attachment 8426025 [details] [diff] [review]
> > (WIP) add-browserapi v8
> > 
> > Review of attachment 8426025 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I have a hard time understanding what the API outlines here
> > <https://wiki.mozilla.org/User:Timdream/mozbrowserSelection> is trying to
> > achieve.  Not sure where the API was discussed, but the wiki page lacks a
> > lot of details on what the API should do (for example it's not clear to me
> > whether this is trying to handle input/textarea, contentEditable regions, or
> > all web content, or a combination of the above.  Plus the patch doesn't even
> > implement what that wiki page describes, so it's really hard to figure out
> > what the intention is, but depending on what we want to achieve, the general
> > approach of the patch may or may not be correct.  I'm going to provide some
> > hints on the things that are obviously wrong in the patch but I need to know
> > more about what we're trying to achieve here to be able to provide better
> > feedback.
> In 2.0, we'll handle element which is editable, that is, input/textarea and
> contenteditable elements.

But why?  Handling contentEditable means that you're going to do all of the work that is required to handle this in the general case.

> > 
> > Note that I didn't look at the parts of the patch that I have not commented
> > on very carefully.
> > 
> > ::: dom/browser-element/BrowserElementChildPreload.js
> > @@ +96,5 @@
> > > +          domRange.startContainer.nodeType == Ci.nsIDOMNode.ELEMENT_NODE &&
> > > +          domRange.startContainer.hasChildNodes() &&
> > > +          domRange.startOffset < domRange.startContainer.childNodes.length &&
> > > +          domRange.startContainer.childNodes[domRange.startOffset].nodeType
> > > +            == Ci.nsIDOMNode.ELEMENT_NODE) {
> > 
> > Not sure what this check is trying to do.
> http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsRange.
> cpp?from=nsRange.cpp#2824
> According to the spec, we'll get empty client rect if range is collapsed and
> the node is not text node.
> So in this case I need get correct client rect by manual.

OK.  Makes sense.

> > @@ +102,5 @@
> > > +        boundingClientRect = element.getBoundingClientRect();
> > > +      }
> > > +
> > > +      let clipboardService = Cc["@mozilla.org/widget/clipboard;1"].
> > > +                             getService(Ci.nsIClipboard);
> > 
> > Question about the general approach.  Why don't we perform the clipboard
> > operation in the child process?
> We actually perform the clipboard operation in the child process. The parent
> process is only responsible for showing utility bubble (such as copy, paste
> button).

Oh, you're right.  I think I confused myself.

> > @@ +123,5 @@
> > > +                     canPaste: clipboardHasData,
> > > +                     show: true,
> > > +                     zoomFactor: zoomFactor};
> > > +
> > > +      let elt = this.getFocusedElement();
> > 
> > It's not clear to me why we're tying things to the focused element.  That is
> > often where the selection is but you could manually move both the focus and
> > the selection in ways that gets them out of sync.  On desktop at least we
> > always try to do copy/paste/etc based on the selection and not which element
> > currently has the focus.
> I'll think about this problem.
> > 
> > @@ +124,5 @@
> > > +                     show: true,
> > > +                     zoomFactor: zoomFactor};
> > > +
> > > +      let elt = this.getFocusedElement();
> > > +      if (elt && elt.editor) {
> > 
> > See my comment below about accessing .editor.
> > 
> > @@ +142,5 @@
> > > +        detail.right += nowRect.left;
> > > +        nowWindow = nowWindow.parent;
> > > +      }
> > > +
> > > +      cachedSelection = selection;
> > 
> > What guarantees that this cached selection is up to date by the time we try
> > to use it?
> > 
> > @@ +143,5 @@
> > > +        nowWindow = nowWindow.parent;
> > > +      }
> > > +
> > > +      cachedSelection = selection;
> > > +      sendAsyncMsg("textualmenu", detail);
> > 
> > I assume this should be "contextualmenu"?
> The wiki page says use "textualmenu"

D'oh, you're right, my eyes just read that as "contextualmenu", sorry about that.

I'm not a native English speaker but to me "textualmenu" doesn't make any sense.  Why did we pick that name?  (And where did this discussion happen?)

> > @@ +148,5 @@
> > > +    }
> > > +  },
> > > +
> > > +  notifySelectionChanged: function(document, selection, reason) {
> > > +    // We only care about mouse up here
> > 
> > Why?
> Because we don't want to show utility bubble when user is changing the
> selection range. We only want to show bubble when user finish selection
> change. Mouse up is somehow a "finish" sign.

OK...

> > @@ +167,5 @@
> > > +  },
> > > +
> > > +  onFocus: function() {
> > > +    let elt = selectionListener.getFocusedElement();
> > > +    if (!elt || !elt.editor) {
> > 
> > Not sure what kinds of elements you're expecting to see here, but I'm
> > guessing this is accessing
> > <http://mxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLInputElement.
> > webidl?force=1#177>, so you seem to be only trying to handle input/textarea.
> > Is that correct?
> > 
> > I really don't think that's the right thing to do...
> Yap, this code handle only input/textarea because I cannot get all
> input/textarea in the beginning because I'll lose input/textarea which is
> dynamic created. So I listen to focus event and check if it has editor or
> not. If it has editor means it is input/textarea so I add a selection
> listener for it.
> 
> Contenteditable element is handled by global selection object. I add
> listener to global selection object when DOMContentLoaded event fired.

OK, what you say here makes sense.  But I still can't make sense of the larger picture.  I think the reason is that I don't yet understand what we're trying to build here and why we're doing things the way that we are.  I suspect that some of the things that you're doing here may already be supported by Gecko so I'd like to avoid reinventing those wheels if we can, but I need to understand what we're trying to achieve here better in order to be able to give you better suggestions.  Can you please help me understand that?

> > @@ +170,5 @@
> > > +    let elt = selectionListener.getFocusedElement();
> > > +    if (!elt || !elt.editor) {
> > > +      return;
> > > +    }
> > > +    let selection = elt.editor.selection;
> > 
> > So depending on what we want to do, this may be the wrong selection object. 
> > We have one global selection object per document and also one selection
> > object per input/textarea.  This code seems to be tailored to disregard the
> > global selection object, and I think that's wrong.
> Mentioned above, onFocus only handled input/textarea.

We shouldn't really need to do any of this...

> > @@ +176,5 @@
> > > +      return;
> > > +    }
> > > +    // Don't add the selection's listener more than once to the same window,
> > > +    // if the selection object is the same
> > > +    if (!("selection" in selections(elt) && selections(elt).selection === selection)) {
> > 
> > This is too much JS for my little brain.  :)  Not sure what this check tries
> > to achieve, but it looks fishy.
> Just avoid to add duplicate selection listener to same element.

Ah, I see.  Hopefully we won't need to add any selection listeners here at all!

> > @@ +182,5 @@
> > > +      // `nsISelectionPrivate` before working on it, in case is `null`.
> > > +      //
> > > +      // If it's `null` it's likely too early to add the listener, and we demand
> > > +      // that operation to `document-shown` - it can easily happens for frames
> > > +      if (selection instanceof Ci.nsISelectionPrivate) {
> > 
> > This will always be true.
> > 
> > @@ +183,5 @@
> > > +      //
> > > +      // If it's `null` it's likely too early to add the listener, and we demand
> > > +      // that operation to `document-shown` - it can easily happens for frames
> > > +      if (selection instanceof Ci.nsISelectionPrivate) {
> > > +        selection.addSelectionListener(selectionListener);
> > 
> > So this sets up one selection listener per input/textarea, but I don't see
> > anywhere where we account for that in this patch, which seems to suggest
> > that this patch should break if you have more than one of those elements on
> > a page.
> > 
> > @@ +715,5 @@
> > > +  },
> > > +
> > > +  _contentLoadedHandler: function(e) {
> > > +    let doc = e.target;
> > > +    let selection = doc.getSelection();
> > 
> > Here you're looking at the global per-document selection object...
> Yes this one add listener to global selection object.
> > 
> > @@ +1181,5 @@
> > > +          cachedSelection.selectAllChildren(cachedSelection.focusNode.parentElement);
> > > +        } else if (cachedSelection.focusNode.nodeType == Ci.nsIDOMNode.ELEMENT_NODE) {
> > > +          cachedSelection.selectAllChildren(cachedSelection.focusNode);
> > > +        }
> > > +      }
> > 
> > Again, I'm not sure what this code is trying to do here, but this looks
> > wrong to me.
> This code is removed in next patch.
> > 
> > @@ +1202,5 @@
> > > +      }
> > > +    }
> > > +  },
> > > +
> > > +  _copyToClipboard: function() {
> > 
> > Why are we inventing these new ways of copying/pasting/etc?  Why not use
> > what we already support in Gecko?
> I'll fix this in next patch.
> > 
> > @@ +1222,5 @@
> > > +            throw new Error("Couldn't set the clipboard due to an internal error " +
> > > +                            "(couldn't create a Transferable object).");
> > > +          }
> > > +          // Bug 769440: Starting with FF16, transferable have to be inited
> > > +          if ("init" in xferable) {
> > 
> > No need to test this.

I'll hold off reviewing the new version of this patch until I understand the goal here better.  I hope that the implementation can still be further simplified.

Comment 24

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #23)
> (In reply to Morris Tseng [:mtseng] from comment #21)
> > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> > #20)
> > > Comment on attachment 8426025 [details] [diff] [review]
> > > (WIP) add-browserapi v8
> > > 
> > > Review of attachment 8426025 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I have a hard time understanding what the API outlines here
> > > <https://wiki.mozilla.org/User:Timdream/mozbrowserSelection> is trying to
> > > achieve.  Not sure where the API was discussed, but the wiki page lacks a
> > > lot of details on what the API should do (for example it's not clear to me
> > > whether this is trying to handle input/textarea, contentEditable regions, or
> > > all web content, or a combination of the above.  Plus the patch doesn't even
> > > implement what that wiki page describes, so it's really hard to figure out
> > > what the intention is, but depending on what we want to achieve, the general
> > > approach of the patch may or may not be correct.  I'm going to provide some
> > > hints on the things that are obviously wrong in the patch but I need to know
> > > more about what we're trying to achieve here to be able to provide better
> > > feedback.
> > In 2.0, we'll handle element which is editable, that is, input/textarea and
> > contenteditable elements.
> 
> But why?  Handling contentEditable means that you're going to do all of the
> work that is required to handle this in the general case.
> 
Because TouchCaret and SelectionCaret appears when user edit editable elements and we need let them able to copy/paste text when TouchCaret and SelectionCaret appears.

> > > 
> > > Note that I didn't look at the parts of the patch that I have not commented
> > > on very carefully.
> > > 
> > > ::: dom/browser-element/BrowserElementChildPreload.js
> > > @@ +96,5 @@
> > > > +          domRange.startContainer.nodeType == Ci.nsIDOMNode.ELEMENT_NODE &&
> > > > +          domRange.startContainer.hasChildNodes() &&
> > > > +          domRange.startOffset < domRange.startContainer.childNodes.length &&
> > > > +          domRange.startContainer.childNodes[domRange.startOffset].nodeType
> > > > +            == Ci.nsIDOMNode.ELEMENT_NODE) {
> > > 
> > > Not sure what this check is trying to do.
> > http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsRange.
> > cpp?from=nsRange.cpp#2824
> > According to the spec, we'll get empty client rect if range is collapsed and
> > the node is not text node.
> > So in this case I need get correct client rect by manual.
> 
> OK.  Makes sense.
> 
> > > @@ +102,5 @@
> > > > +        boundingClientRect = element.getBoundingClientRect();
> > > > +      }
> > > > +
> > > > +      let clipboardService = Cc["@mozilla.org/widget/clipboard;1"].
> > > > +                             getService(Ci.nsIClipboard);
> > > 
> > > Question about the general approach.  Why don't we perform the clipboard
> > > operation in the child process?
> > We actually perform the clipboard operation in the child process. The parent
> > process is only responsible for showing utility bubble (such as copy, paste
> > button).
> 
> Oh, you're right.  I think I confused myself.
> 
> > > @@ +123,5 @@
> > > > +                     canPaste: clipboardHasData,
> > > > +                     show: true,
> > > > +                     zoomFactor: zoomFactor};
> > > > +
> > > > +      let elt = this.getFocusedElement();
> > > 
> > > It's not clear to me why we're tying things to the focused element.  That is
> > > often where the selection is but you could manually move both the focus and
> > > the selection in ways that gets them out of sync.  On desktop at least we
> > > always try to do copy/paste/etc based on the selection and not which element
> > > currently has the focus.
> > I'll think about this problem.
> > > 
> > > @@ +124,5 @@
> > > > +                     show: true,
> > > > +                     zoomFactor: zoomFactor};
> > > > +
> > > > +      let elt = this.getFocusedElement();
> > > > +      if (elt && elt.editor) {
> > > 
> > > See my comment below about accessing .editor.
> > > 
> > > @@ +142,5 @@
> > > > +        detail.right += nowRect.left;
> > > > +        nowWindow = nowWindow.parent;
> > > > +      }
> > > > +
> > > > +      cachedSelection = selection;
> > > 
> > > What guarantees that this cached selection is up to date by the time we try
> > > to use it?
> > > 
> > > @@ +143,5 @@
> > > > +        nowWindow = nowWindow.parent;
> > > > +      }
> > > > +
> > > > +      cachedSelection = selection;
> > > > +      sendAsyncMsg("textualmenu", detail);
> > > 
> > > I assume this should be "contextualmenu"?
> > The wiki page says use "textualmenu"
> 
> D'oh, you're right, my eyes just read that as "contextualmenu", sorry about
> that.
> 
> I'm not a native English speaker but to me "textualmenu" doesn't make any
> sense.  Why did we pick that name?  (And where did this discussion happen?)
> 
I didn't follow the discussion as well, ni timdream for naming explanation.

> > > @@ +148,5 @@
> > > > +    }
> > > > +  },
> > > > +
> > > > +  notifySelectionChanged: function(document, selection, reason) {
> > > > +    // We only care about mouse up here
> > > 
> > > Why?
> > Because we don't want to show utility bubble when user is changing the
> > selection range. We only want to show bubble when user finish selection
> > change. Mouse up is somehow a "finish" sign.
> 
> OK...
> 
> > > @@ +167,5 @@
> > > > +  },
> > > > +
> > > > +  onFocus: function() {
> > > > +    let elt = selectionListener.getFocusedElement();
> > > > +    if (!elt || !elt.editor) {
> > > 
> > > Not sure what kinds of elements you're expecting to see here, but I'm
> > > guessing this is accessing
> > > <http://mxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLInputElement.
> > > webidl?force=1#177>, so you seem to be only trying to handle input/textarea.
> > > Is that correct?
> > > 
> > > I really don't think that's the right thing to do...
> > Yap, this code handle only input/textarea because I cannot get all
> > input/textarea in the beginning because I'll lose input/textarea which is
> > dynamic created. So I listen to focus event and check if it has editor or
> > not. If it has editor means it is input/textarea so I add a selection
> > listener for it.
> > 
> > Contenteditable element is handled by global selection object. I add
> > listener to global selection object when DOMContentLoaded event fired.
> 
> OK, what you say here makes sense.  But I still can't make sense of the
> larger picture.  I think the reason is that I don't yet understand what
> we're trying to build here and why we're doing things the way that we are. 
> I suspect that some of the things that you're doing here may already be
> supported by Gecko so I'd like to avoid reinventing those wheels if we can,
> but I need to understand what we're trying to achieve here better in order
> to be able to give you better suggestions.  Can you please help me
> understand that?
> 
The way we implemeneted Touch/Selection Caret also similar like this. Those carets added self to the selection listener as well.
Add-on's selection sdk also do similar way. http://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/selection.js#341
I have no idea is there a simple way to do this. roc, do you have any idea?

Comment 25

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Morris Tseng [:mtseng] from comment #24)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #23)
> > I'm not a native English speaker but to me "textualmenu" doesn't make any
> > sense.  Why did we pick that name?  (And where did this discussion happen?)
> > 
> I didn't follow the discussion as well, ni timdream for naming explanation.
> 

The discussions happened offline (sorry!) a few times in MV and Taipei. 

If you look at the edit log of the wiki page, it was named "mozbrowserselectionstart" etc, but Alive and I then realized we need the pop-up menu to show, not just when the selection actually starts, according to UX spec. Hence we come up with a name that is more explicit as in "Gecko need the embedder to show a textual operation menu". That's not the best name, and if we have a better one from a native English speaker we can certainly modify it.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not sure what's being asked here. AFAIK users of the mozbrowser API don't have access to the <iframe>'s subdocument so they can't add their own selection listeners.

Comment 27

[(no longer active)]

Thanks for the response, Tim.  About the naming issue, I still don't really understand what this event is supposed to represent exactly, but let's first talk about the intention behind the API.  Since the discussion has not been documented, I'm going to guesswork my way here, please let me know if what I'm assuming below doesn't match the reality!

As far as the selection UI is concerned, that should all be handled by Gecko, so I don't think we need to worry about that in this API (please correct me if you disagree!).

It seems like one reason why mozbrowsertextualmenu exists is to allow the embedder to know whether they can initiate a cut/copy/paste action.  Gecko already keeps track of whether each one of these commands is enabled by default, and I think we can use the same infrastructure as opposed to approximate that information based on the selection.  The way to use that infrastructure is to look at the |controllers| property of Window, HTMLInputElement and HTMLTextAreaElement (they're all a chrome-only property of course).  That will give you an nsIControllers object which you can use to get to the individual commands (such as cmd_copy, cmd_paste etc.)  We have this code <http://mxr.mozilla.org/mozilla-central/source/toolkit/content/globalOverlay.js#71> which updates the commands by looking at the controller for each command.  We have some XUL specific functionality built around this (look at nsIDOMXULCommandDispatcher and friends) and also some support to register commandupdater elements (see nsXULContentUtils::SetCommandUpdater).  To see how this all fits together with XUL, check out editMenuOverlay.xul and look at the commandupdater elements there and check out the oncommandupdate event handler.

We should really be using this infrastructure, and fill out the missing bits which won't be available in non-XUL documents.  The basic idea is to use the controllers for input/textarea if one is focused, otherwise look at the controllers of the window and figure out whether a given command is enabled there or not.

For boundingClientRect, I'm not really sure what the use case is, and when we want to expose this, etc.  Can someone please provide some details on that?

Instead of invididual methods for cut/copy/paste/selectall, we may just want to provide a way for the embedder to run commands, and then map them to our internal commands (cmd_copy, etc.).  We do something similar to this for the document.execCommand API.

Please let me know what you think about the above.

Comment 28

[(no longer active)]

Also, this is the kind of topic which would have been best discussed on dev-webapi before we moved to the implementation.  :-)

Comment 29

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #27)
> Thanks for the response, Tim.  About the naming issue, I still don't really
> understand what this event is supposed to represent exactly, but let's first
> talk about the intention behind the API.  Since the discussion has not been
> documented, I'm going to guesswork my way here, please let me know if what
> I'm assuming below doesn't match the reality!
> 
> As far as the selection UI is concerned, that should all be handled by
> Gecko, so I don't think we need to worry about that in this API (please
> correct me if you disagree!).
We decide to show utility bubble from embedder because this give us more flexibility for customizing utility bubble. Thus, part of UI is handled by Gecko and part of UI is handled by embedder. That why we need this API for informing embedder to show utility bubble.

So the implementation is divided into 2 parts:
1. Listen to selection change event. Once the selection change is occurred, send "mozbrowsertextualmenu" which contains necessary information to embedder.
2. When embedder initiate cut/copy/paste/selectall action, embedder just inform us and we do the actually action.
> 
> It seems like one reason why mozbrowsertextualmenu exists is to allow the
> embedder to know whether they can initiate a cut/copy/paste action.  Gecko
As mentioned above, this api not only initiate actions but also inform embedder to show utility bubble.

> already keeps track of whether each one of these commands is enabled by
> default, and I think we can use the same infrastructure as opposed to
> approximate that information based on the selection.  The way to use that
> infrastructure is to look at the |controllers| property of Window,
> HTMLInputElement and HTMLTextAreaElement (they're all a chrome-only property
> of course).  That will give you an nsIControllers object which you can use
> to get to the individual commands (such as cmd_copy, cmd_paste etc.)  We
> have this code
> <http://mxr.mozilla.org/mozilla-central/source/toolkit/content/globalOverlay.
> js#71> which updates the commands by looking at the controller for each
> command.  We have some XUL specific functionality built around this (look at
> nsIDOMXULCommandDispatcher and friends) and also some support to register
> commandupdater elements (see nsXULContentUtils::SetCommandUpdater).  To see
> how this all fits together with XUL, check out editMenuOverlay.xul and look
> at the commandupdater elements there and check out the oncommandupdate event
> handler.
> 
> We should really be using this infrastructure, and fill out the missing bits
> which won't be available in non-XUL documents.  The basic idea is to use the
> controllers for input/textarea if one is focused, otherwise look at the
> controllers of the window and figure out whether a given command is enabled
> there or not.
Thanks for the feedback, I have updated my patch to use controllers for selectall/cut/copy/paste actions. I'll update my patch later.
> 
> For boundingClientRect, I'm not really sure what the use case is, and when
> we want to expose this, etc.  Can someone please provide some details on
> that?
https://bugzilla.mozilla.org/show_bug.cgi?id=921965
According to the UX spec, page 15 describes in different situation we should place utility bubble in different place. So we need boundingClientRect to tell embedder where we are current select at so that embedder could place utility bubble to right position.
> 
> Instead of invididual methods for cut/copy/paste/selectall, we may just want
> to provide a way for the embedder to run commands, and then map them to our
> internal commands (cmd_copy, etc.).  We do something similar to this for the
> document.execCommand API.
> 
> Please let me know what you think about the above.

The implementation detail part 1 shows we need add selection listener in order to listen selection change event. Do you have any easier way to listen this event? Thanks

Comment 30

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: (WIP) add-browserapi v10

Using controllers to do cut/copy/paste/selectall action.

Comment 31

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I think Morris made a great recap on why API was wrapped around that way (Thanks!) so I won't repeat here.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #27)
> Thanks for the response, Tim.  

No worries, and sorry again for falling through the crack on bringing this up to the mailing list. Speak of the API design, I may have been designing blind without considering that we have in Gecko already.

Please loop me if you have new ideas around the API and tell me to post this to webapi mailing list if I need to.

Comment 32

[(no longer active)]

Sorry, Morris and Tim, I didn't get a chance to look at this today.  I'll look tomorrow -- keeping the ni? flag until I do that.

Comment 33

[(no longer active)]

So sorry about failing to keep my promise in comment 32.  :(

(In reply to Morris Tseng [:mtseng] from comment #29)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #27)
> > Thanks for the response, Tim.  About the naming issue, I still don't really
> > understand what this event is supposed to represent exactly, but let's first
> > talk about the intention behind the API.  Since the discussion has not been
> > documented, I'm going to guesswork my way here, please let me know if what
> > I'm assuming below doesn't match the reality!
> > 
> > As far as the selection UI is concerned, that should all be handled by
> > Gecko, so I don't think we need to worry about that in this API (please
> > correct me if you disagree!).
> We decide to show utility bubble from embedder because this give us more
> flexibility for customizing utility bubble. Thus, part of UI is handled by
> Gecko and part of UI is handled by embedder. That why we need this API for
> informing embedder to show utility bubble.

OK, that makes sense.  BTW I'm going to use https://bug921965.bugzilla.mozilla.org/attachment.cgi?id=8412542 as the UX spec (coming from bug 921965 which you linked to below -- thanks for that!)

> So the implementation is divided into 2 parts:
> 1. Listen to selection change event. Once the selection change is occurred,
> send "mozbrowsertextualmenu" which contains necessary information to
> embedder.

And I assume we're only interested by selection changes made by the user, not the ones made by let's say the page script, right?

> 2. When embedder initiate cut/copy/paste/selectall action, embedder just
> inform us and we do the actually action.

Makes sense.

> > It seems like one reason why mozbrowsertextualmenu exists is to allow the
> > embedder to know whether they can initiate a cut/copy/paste action.  Gecko
> As mentioned above, this api not only initiate actions but also inform
> embedder to show utility bubble.

OK, I think I'm understanding things better now...

> > already keeps track of whether each one of these commands is enabled by
> > default, and I think we can use the same infrastructure as opposed to
> > approximate that information based on the selection.  The way to use that
> > infrastructure is to look at the |controllers| property of Window,
> > HTMLInputElement and HTMLTextAreaElement (they're all a chrome-only property
> > of course).  That will give you an nsIControllers object which you can use
> > to get to the individual commands (such as cmd_copy, cmd_paste etc.)  We
> > have this code
> > <http://mxr.mozilla.org/mozilla-central/source/toolkit/content/globalOverlay.
> > js#71> which updates the commands by looking at the controller for each
> > command.  We have some XUL specific functionality built around this (look at
> > nsIDOMXULCommandDispatcher and friends) and also some support to register
> > commandupdater elements (see nsXULContentUtils::SetCommandUpdater).  To see
> > how this all fits together with XUL, check out editMenuOverlay.xul and look
> > at the commandupdater elements there and check out the oncommandupdate event
> > handler.
> > 
> > We should really be using this infrastructure, and fill out the missing bits
> > which won't be available in non-XUL documents.  The basic idea is to use the
> > controllers for input/textarea if one is focused, otherwise look at the
> > controllers of the window and figure out whether a given command is enabled
> > there or not.
> Thanks for the feedback, I have updated my patch to use controllers for
> selectall/cut/copy/paste actions. I'll update my patch later.

Great.  FWIW I think we should also try to hook up the infrastructure to get Gecko to tell us when the state of these commands change.  I'll write more about that below.

> > For boundingClientRect, I'm not really sure what the use case is, and when
> > we want to expose this, etc.  Can someone please provide some details on
> > that?
> https://bugzilla.mozilla.org/show_bug.cgi?id=921965
> According to the UX spec, page 15 describes in different situation we should
> place utility bubble in different place. So we need boundingClientRect to
> tell embedder where we are current select at so that embedder could place
> utility bubble to right position.

OK, makes sense.

> > Instead of invididual methods for cut/copy/paste/selectall, we may just want
> > to provide a way for the embedder to run commands, and then map them to our
> > internal commands (cmd_copy, etc.).  We do something similar to this for the
> > document.execCommand API.
> > 
> > Please let me know what you think about the above.
> 
> The implementation detail part 1 shows we need add selection listener in
> order to listen selection change event. Do you have any easier way to listen
> this event? Thanks

Yes, I think I do.  :-)

nsGlobalWindow::UpdateCommands is the central place in Gecko which dispatches update notifications used by XUL updaters to drive updating things such as copy/paste elements in the UI.  <https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/Updating_Commands> might be an interesting read on how this works in the XUL world (see the "Command Updaters" section in particular.)  I think we need to use that infrastructure.

So here's an API proposal.  Please criticize it fiercely.  :-)

enum BrowserCommand {
  "cut",
  "copy",
  "paste",
  "selectall",
};

dictionary SelectionChangeEventInit : EventInit {
  DOMString? selectedText = "";
  DOMRectReadOnly? boundingClientRect = null;
};

[Constructor(DOMString type, optional SelectionChangeEventInit eventInit)]
interface SelectionChangeEvent : Event {
  readonly attribute DOMString? selectedText;
  readonly attribute DOMRectReadOnly? boundingClientRect;
};

// The event passed to this handler is SelectionChangeEvent
attribute EventHandler onselectionchange;

boolean isCommandEnabled(BrowserCommand command);
void doCommand(BrowserCommand command);

For the implementation, isCommandEnabled and doCommand map very cleanly to our internal command structure, similar to what you currently have in your patch.  Please note that you need to map BrowserCommand names to our internal names.

The more interesting part of the implementation would be onselectionchange.  Instead of hooking into selection listeners manually, I think we should use nsGlobalWindow::UpdateCommands, where it is passed "select" (which is one of the four verbs that it supports, which are documented in the above MDN page.)  Right now, UpdateCommands only receives the string argument, and we need to extend it to take the rect in the case of "select".  I think internally you can extend that method to receive an nsISelection* (which can be null for other verbs of course) and get the rect and selected text from the selection object.  You'll need to extend nsTextInputListener::NotifySelectionChanged and nsDocViewerSelectionListener::NotifySelectionChanged for the case of text control selections and the global selection object respectively.  That way you can get rid of most of the code in the current patch which tries to do the right thing with the selection controllers.

Please let me know what you think!

Comment 34

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #33)
> So here's an API proposal.  Please criticize it fiercely.  :-)

Ehsan, looking at the IDL(?) alone, I don't think there is a major differences so I think it's fine (Morris can disagree :D). However there are some detail that is unclear to me and I want to make sure there are no major changes on these parts.

* You are still talking about method/event on mozbrowser iframe right? I assume this part did not change.
* If you look at the wiki log of my page you will indeed find a mozbrowserselectionchange event that got removed. The major reason of that change is because Gaia need Gecko to send events according to spec (sometimes, long press input box w/o actually selecting something), not just selection change. Do you aware of that when proposing the API? Not entirely sure you are thinking about changing the timing of the event or not.
* I assume doCommand() is about exposing something simular document.execCommand() to the embedder? How about simply name it execCommand()?
https://developer.mozilla.org/en-US/docs/Web/API/document.execCommand

Comment 35

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #33)
> So sorry about failing to keep my promise in comment 32.  :(
> 
> (In reply to Morris Tseng [:mtseng] from comment #29)
> > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> > #27)
> > So the implementation is divided into 2 parts:
> > 1. Listen to selection change event. Once the selection change is occurred,
> > send "mozbrowsertextualmenu" which contains necessary information to
> > embedder.
> 
> And I assume we're only interested by selection changes made by the user,
> not the ones made by let's say the page script, right?
Exactly.

> 
> nsGlobalWindow::UpdateCommands is the central place in Gecko which
> dispatches update notifications used by XUL updaters to drive updating
> things such as copy/paste elements in the UI. 
> <https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/
> Updating_Commands> might be an interesting read on how this works in the XUL
> world (see the "Command Updaters" section in particular.)  I think we need
> to use that infrastructure.
> 
> So here's an API proposal.  Please criticize it fiercely.  :-)
I'm a bit confused here.

I think this API is for detecting selection change and executing command, not for informing embedder to show bubble. Inform event still use mozbrowsertextualmenu as wiki described. Am I correct?
> 
> enum BrowserCommand {
>   "cut",
>   "copy",
>   "paste",
>   "selectall",
> };
> 
> dictionary SelectionChangeEventInit : EventInit {
>   DOMString? selectedText = "";
>   DOMRectReadOnly? boundingClientRect = null;
> };
> 
> [Constructor(DOMString type, optional SelectionChangeEventInit eventInit)]
> interface SelectionChangeEvent : Event {
>   readonly attribute DOMString? selectedText;
>   readonly attribute DOMRectReadOnly? boundingClientRect;
> };
Above is clear. 
> 
> // The event passed to this handler is SelectionChangeEvent
> attribute EventHandler onselectionchange;
I don't know where is this EventHandler should be placed at. My guess is in nsIDOMWindow thus we can use window for detecting selection change. right?
> 
> boolean isCommandEnabled(BrowserCommand command);
> void doCommand(BrowserCommand command);
http://dxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLDocument.webidl#53
Since document has execCommand and queryCommandEnabled, should we need those 2 functions which look the same purpose. And also if we need those 2 functions, where are they be place at? I didn't understand this very well. Can you tell me more details about here? Thanks.

> 
> For the implementation, isCommandEnabled and doCommand map very cleanly to
> our internal command structure, similar to what you currently have in your
> patch.  Please note that you need to map BrowserCommand names to our
> internal names.
> 
> The more interesting part of the implementation would be onselectionchange. 
> Instead of hooking into selection listeners manually, I think we should use
> nsGlobalWindow::UpdateCommands, where it is passed "select" (which is one of
> the four verbs that it supports, which are documented in the above MDN
> page.)  Right now, UpdateCommands only receives the string argument, and we
> need to extend it to take the rect in the case of "select".  I think
> internally you can extend that method to receive an nsISelection* (which can
> be null for other verbs of course) and get the rect and selected text from
> the selection object.  You'll need to extend
> nsTextInputListener::NotifySelectionChanged and
> nsDocViewerSelectionListener::NotifySelectionChanged for the case of text
> control selections and the global selection object respectively.  That way
> you can get rid of most of the code in the current patch which tries to do
> the right thing with the selection controllers.
What I understand here is nsTextInputListener::NotifySelectChanged and nsDocViewerSelectionListener::NotifySelectionChange call nsGlobalWindow::UpdateCommands with 2 argument which are string "select" and nsISelection* object. Then nsGlobalWindow::UpdateCommands will dispatch SelectionChangeEvent as above described. So we can just listen to this event and get rid of my most code in my patch. Is it right?
> 
> Please let me know what you think!

Comment 36

[(no longer active)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #34)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #33)
> > So here's an API proposal.  Please criticize it fiercely.  :-)
> 
> Ehsan, looking at the IDL(?) alone, I don't think there is a major
> differences so I think it's fine (Morris can disagree :D). However there are
> some detail that is unclear to me and I want to make sure there are no major
> changes on these parts.
> 
> * You are still talking about method/event on mozbrowser iframe right? I
> assume this part did not change.

Yes.

> * If you look at the wiki log of my page you will indeed find a
> mozbrowserselectionchange event that got removed. The major reason of that
> change is because Gaia need Gecko to send events according to spec
> (sometimes, long press input box w/o actually selecting something), not just
> selection change. Do you aware of that when proposing the API? Not entirely
> sure you are thinking about changing the timing of the event or not.

Hmm, according to which spec?  The events that I'm talking about are internal events which are never visible to Web content.

> * I assume doCommand() is about exposing something simular
> document.execCommand() to the embedder? How about simply name it
> execCommand()?
> https://developer.mozilla.org/en-US/docs/Web/API/document.execCommand

I actually picked a different name intentionally, since even though it will be similar in concept, the commands are probably going to be different, so I wanted to avoid the confusion when someone would think that doCommand should work similarly to execCommand.

Comment 37

[(no longer active)]

(In reply to Morris Tseng [:mtseng] from comment #35)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #33)
> > So sorry about failing to keep my promise in comment 32.  :(
> > 
> > (In reply to Morris Tseng [:mtseng] from comment #29)
> > > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> > > #27)
> > > So the implementation is divided into 2 parts:
> > > 1. Listen to selection change event. Once the selection change is occurred,
> > > send "mozbrowsertextualmenu" which contains necessary information to
> > > embedder.
> > 
> > And I assume we're only interested by selection changes made by the user,
> > not the ones made by let's say the page script, right?
> Exactly.

Oh, so I forgot to incorporate this into the API proposal.  Sorry about that!  I meant to add an enum SelectionReason to reflect these values <http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsISelectionListener.idl#14> as a member of the event interface.

> > nsGlobalWindow::UpdateCommands is the central place in Gecko which
> > dispatches update notifications used by XUL updaters to drive updating
> > things such as copy/paste elements in the UI. 
> > <https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/
> > Updating_Commands> might be an interesting read on how this works in the XUL
> > world (see the "Command Updaters" section in particular.)  I think we need
> > to use that infrastructure.
> > 
> > So here's an API proposal.  Please criticize it fiercely.  :-)
> I'm a bit confused here.
> 
> I think this API is for detecting selection change and executing command,
> not for informing embedder to show bubble. Inform event still use
> mozbrowsertextualmenu as wiki described. Am I correct?

Yes, you're right about the API.  We should keep the current UX spec and ensure that the API will satisfy the UX spec, but I don't think that we need to design the API in a way that can only satisfy the current UX spec, because then the next time that the desired UX changes, we'd need to change the API.

Based on that, my idea was to use the selectionchange event to make the decision on when to show the bubble at the embedder side.  Basically, the embedder will monitor the status of the selection in the mozbrowser hosted content and will show a bubble when it detects that a new non-empty selection was created because of a mouse-up event, and then it would hide the bubble when it detects that the selection was changed to be empty again.  It could also use the selectionchange events' rect to move the bubble when the selection moves.  This way, we can use the same API to implement other possible UX specs in the future.

Do you think that makes sense?

> > enum BrowserCommand {
> >   "cut",
> >   "copy",
> >   "paste",
> >   "selectall",
> > };
> > 
> > dictionary SelectionChangeEventInit : EventInit {
> >   DOMString? selectedText = "";
> >   DOMRectReadOnly? boundingClientRect = null;
> > };
> > 
> > [Constructor(DOMString type, optional SelectionChangeEventInit eventInit)]
> > interface SelectionChangeEvent : Event {
> >   readonly attribute DOMString? selectedText;
> >   readonly attribute DOMRectReadOnly? boundingClientRect;
> > };
> Above is clear. 
> > 
> > // The event passed to this handler is SelectionChangeEvent
> > attribute EventHandler onselectionchange;
> I don't know where is this EventHandler should be placed at. My guess is in
> nsIDOMWindow thus we can use window for detecting selection change. right?

No, that was intended to go on mozbrowser iframes, as well as isCommandEnabled and doCommand below.

> > boolean isCommandEnabled(BrowserCommand command);
> > void doCommand(BrowserCommand command);
> http://dxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLDocument.
> webidl#53
> Since document has execCommand and queryCommandEnabled, should we need those
> 2 functions which look the same purpose. And also if we need those 2
> functions, where are they be place at? I didn't understand this very well.
> Can you tell me more details about here? Thanks.

See comment 36 please.  I used a different name intentionally.

> > For the implementation, isCommandEnabled and doCommand map very cleanly to
> > our internal command structure, similar to what you currently have in your
> > patch.  Please note that you need to map BrowserCommand names to our
> > internal names.
> > 
> > The more interesting part of the implementation would be onselectionchange. 
> > Instead of hooking into selection listeners manually, I think we should use
> > nsGlobalWindow::UpdateCommands, where it is passed "select" (which is one of
> > the four verbs that it supports, which are documented in the above MDN
> > page.)  Right now, UpdateCommands only receives the string argument, and we
> > need to extend it to take the rect in the case of "select".  I think
> > internally you can extend that method to receive an nsISelection* (which can
> > be null for other verbs of course) and get the rect and selected text from
> > the selection object.  You'll need to extend
> > nsTextInputListener::NotifySelectionChanged and
> > nsDocViewerSelectionListener::NotifySelectionChanged for the case of text
> > control selections and the global selection object respectively.  That way
> > you can get rid of most of the code in the current patch which tries to do
> > the right thing with the selection controllers.
> What I understand here is nsTextInputListener::NotifySelectChanged and
> nsDocViewerSelectionListener::NotifySelectionChange call
> nsGlobalWindow::UpdateCommands with 2 argument which are string "select" and
> nsISelection* object.

Well, they currently just pass the string, so we'd need to add the second argument, but yes.

> Then nsGlobalWindow::UpdateCommands will dispatch
> SelectionChangeEvent as above described. So we can just listen to this event
> and get rid of my most code in my patch. Is it right?

Yes, exactly!

But I think we should first make sure that we all agree on the API before moving to the implementation, to make sure that you don't have to keep throwing away your patches if we decide to change something in the API.  :-)

Thanks!

Comment 38

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #37)
> (In reply to Morris Tseng [:mtseng] from comment #35)
> > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> > #33)
> > > So sorry about failing to keep my promise in comment 32.  :(
> > > 
> > > (In reply to Morris Tseng [:mtseng] from comment #29)
> > > > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> > > > #27)
> > > > So the implementation is divided into 2 parts:
> > > > 1. Listen to selection change event. Once the selection change is occurred,
> > > > send "mozbrowsertextualmenu" which contains necessary information to
> > > > embedder.
> > > 
> > > And I assume we're only interested by selection changes made by the user,
> > > not the ones made by let's say the page script, right?
> > Exactly.
> 
> Oh, so I forgot to incorporate this into the API proposal.  Sorry about
> that!  I meant to add an enum SelectionReason to reflect these values
> <http://mxr.mozilla.org/mozilla-central/source/content/base/public/
> nsISelectionListener.idl#14> as a member of the event interface.
> 
> > > nsGlobalWindow::UpdateCommands is the central place in Gecko which
> > > dispatches update notifications used by XUL updaters to drive updating
> > > things such as copy/paste elements in the UI. 
> > > <https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/
> > > Updating_Commands> might be an interesting read on how this works in the XUL
> > > world (see the "Command Updaters" section in particular.)  I think we need
> > > to use that infrastructure.
> > > 
> > > So here's an API proposal.  Please criticize it fiercely.  :-)
> > I'm a bit confused here.
> > 
> > I think this API is for detecting selection change and executing command,
> > not for informing embedder to show bubble. Inform event still use
> > mozbrowsertextualmenu as wiki described. Am I correct?
> 
> Yes, you're right about the API.  We should keep the current UX spec and
> ensure that the API will satisfy the UX spec, but I don't think that we need
> to design the API in a way that can only satisfy the current UX spec,
> because then the next time that the desired UX changes, we'd need to change
> the API.
> 
> Based on that, my idea was to use the selectionchange event to make the
> decision on when to show the bubble at the embedder side.  Basically, the
> embedder will monitor the status of the selection in the mozbrowser hosted
> content and will show a bubble when it detects that a new non-empty
> selection was created because of a mouse-up event, and then it would hide
> the bubble when it detects that the selection was changed to be empty again.
> It could also use the selectionchange events' rect to move the bubble when
> the selection moves.  This way, we can use the same API to implement other
> possible UX specs in the future.
> 
> Do you think that makes sense?
Yes, this makes sense. There is one thing that is we're also care about empty selection because if user want to paste text to specific location, user must tap to that location first and this action create a selection change event with empty selection.

I have another scenario about bubble menu close. If user scrolling the view or changing selection by using selection caret, the bubble should close (described in spec page 4 figure 3 and page 17 figure 2). Right now I use a dom event which called "TextualMenuClose" to indicate bubble close. You can find it in my patch. I think we can integrate this event with this api by add a attribute to SelectionChangeEvent, such as "isShowMenu"? Is it make sense?
> 
> > > enum BrowserCommand {
> > >   "cut",
> > >   "copy",
> > >   "paste",
> > >   "selectall",
> > > };
> > > 
> > > dictionary SelectionChangeEventInit : EventInit {
> > >   DOMString? selectedText = "";
> > >   DOMRectReadOnly? boundingClientRect = null;
> > > };
> > > 
> > > [Constructor(DOMString type, optional SelectionChangeEventInit eventInit)]
> > > interface SelectionChangeEvent : Event {
> > >   readonly attribute DOMString? selectedText;
> > >   readonly attribute DOMRectReadOnly? boundingClientRect;
> > > };
> > Above is clear. 
> > > 
> > > // The event passed to this handler is SelectionChangeEvent
> > > attribute EventHandler onselectionchange;
> > I don't know where is this EventHandler should be placed at. My guess is in
> > nsIDOMWindow thus we can use window for detecting selection change. right?
> 
> No, that was intended to go on mozbrowser iframes, as well as
> isCommandEnabled and doCommand below.
Sorry, I'm not very familiar with webidl stuff. Could you tell me mozbrowser iframes belong to which webidl interface? Thanks.
> 
> > > boolean isCommandEnabled(BrowserCommand command);
> > > void doCommand(BrowserCommand command);
> > http://dxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLDocument.
> > webidl#53
> > Since document has execCommand and queryCommandEnabled, should we need those
> > 2 functions which look the same purpose. And also if we need those 2
> > functions, where are they be place at? I didn't understand this very well.
> > Can you tell me more details about here? Thanks.
> 
> See comment 36 please.  I used a different name intentionally.
OK! I see.
> 
> > > For the implementation, isCommandEnabled and doCommand map very cleanly to
> > > our internal command structure, similar to what you currently have in your
> > > patch.  Please note that you need to map BrowserCommand names to our
> > > internal names.
> > > 
> > > The more interesting part of the implementation would be onselectionchange. 
> > > Instead of hooking into selection listeners manually, I think we should use
> > > nsGlobalWindow::UpdateCommands, where it is passed "select" (which is one of
> > > the four verbs that it supports, which are documented in the above MDN
> > > page.)  Right now, UpdateCommands only receives the string argument, and we
> > > need to extend it to take the rect in the case of "select".  I think
> > > internally you can extend that method to receive an nsISelection* (which can
> > > be null for other verbs of course) and get the rect and selected text from
> > > the selection object.  You'll need to extend
> > > nsTextInputListener::NotifySelectionChanged and
> > > nsDocViewerSelectionListener::NotifySelectionChanged for the case of text
> > > control selections and the global selection object respectively.  That way
> > > you can get rid of most of the code in the current patch which tries to do
> > > the right thing with the selection controllers.
> > What I understand here is nsTextInputListener::NotifySelectChanged and
> > nsDocViewerSelectionListener::NotifySelectionChange call
> > nsGlobalWindow::UpdateCommands with 2 argument which are string "select" and
> > nsISelection* object.
> 
> Well, they currently just pass the string, so we'd need to add the second
> argument, but yes.
> 
> > Then nsGlobalWindow::UpdateCommands will dispatch
> > SelectionChangeEvent as above described. So we can just listen to this event
> > and get rid of my most code in my patch. Is it right?
> 
> Yes, exactly!
> 
> But I think we should first make sure that we all agree on the API before
> moving to the implementation, to make sure that you don't have to keep
> throwing away your patches if we decide to change something in the API.  :-)
> 
> Thanks!
Sure!

Comment 39

[(no longer active)]

(In reply to Morris Tseng [:mtseng] from comment #38)
> > Yes, you're right about the API.  We should keep the current UX spec and
> > ensure that the API will satisfy the UX spec, but I don't think that we need
> > to design the API in a way that can only satisfy the current UX spec,
> > because then the next time that the desired UX changes, we'd need to change
> > the API.
> > 
> > Based on that, my idea was to use the selectionchange event to make the
> > decision on when to show the bubble at the embedder side.  Basically, the
> > embedder will monitor the status of the selection in the mozbrowser hosted
> > content and will show a bubble when it detects that a new non-empty
> > selection was created because of a mouse-up event, and then it would hide
> > the bubble when it detects that the selection was changed to be empty again.
> > It could also use the selectionchange events' rect to move the bubble when
> > the selection moves.  This way, we can use the same API to implement other
> > possible UX specs in the future.
> > 
> > Do you think that makes sense?
> Yes, this makes sense. There is one thing that is we're also care about
> empty selection because if user want to paste text to specific location,
> user must tap to that location first and this action create a selection
> change event with empty selection.

You can look at selectedText and compare it against an empty string for that purpose, right?

> I have another scenario about bubble menu close. If user scrolling the view
> or changing selection by using selection caret, the bubble should close
> (described in spec page 4 figure 3 and page 17 figure 2). Right now I use a
> dom event which called "TextualMenuClose" to indicate bubble close. You can
> find it in my patch. I think we can integrate this event with this api by
> add a attribute to SelectionChangeEvent, such as "isShowMenu"? Is it make
> sense?

Hmm, let me see if I understand this correctly.  For the part about page 4 figure 3, we're extending the selection by moving the selection handles, right?  In which case, I would expect the code would keep handling the selectionchange events that the content loaded inside mozbrowser would keep sending and keep the bubble visible as long as the selectedText is not empty.  For page 17 figure 2, you'd see that the selectedText is set to an empty string and you would hide the bubble.  Something like this on the embedder side:

  var bubbleVisible = false;
  function selectionChangeHandler(event) {
    if (bubbleVisible && event.selectedText == "") {
      hideBubble();
      bubbleVisible = false;
    } else if (!bubbleVisible && event.selectedText != "") {
      showBubble();
      bubbleVisible = true;
    }
  }

I think that should work.  What do you think?

> > No, that was intended to go on mozbrowser iframes, as well as
> > isCommandEnabled and doCommand below.
> Sorry, I'm not very familiar with webidl stuff. Could you tell me mozbrowser
> iframes belong to which webidl interface? Thanks.

mozbrowser is unfortunately not ported to WebIDL yet.  I was just using WebIDL as a language to explain the API.  The mozbrowser functions should probably be added like this: <http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.jsm#152>

That being said, you should be able to define SelectionChangeEvent in WebIDL.  I think there is a way to generate an event class, please check with smaug on how that works.  Hopefully you won't have to write any C++ code for the event class.

Comment 40

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #39)
> (In reply to Morris Tseng [:mtseng] from comment #38)
> > > Yes, you're right about the API.  We should keep the current UX spec and
> > > ensure that the API will satisfy the UX spec, but I don't think that we need
> > > to design the API in a way that can only satisfy the current UX spec,
> > > because then the next time that the desired UX changes, we'd need to change
> > > the API.
> > > 
> > > Based on that, my idea was to use the selectionchange event to make the
> > > decision on when to show the bubble at the embedder side.  Basically, the
> > > embedder will monitor the status of the selection in the mozbrowser hosted
> > > content and will show a bubble when it detects that a new non-empty
> > > selection was created because of a mouse-up event, and then it would hide
> > > the bubble when it detects that the selection was changed to be empty again.
> > > It could also use the selectionchange events' rect to move the bubble when
> > > the selection moves.  This way, we can use the same API to implement other
> > > possible UX specs in the future.
> > > 
> > > Do you think that makes sense?
> > Yes, this makes sense. There is one thing that is we're also care about
> > empty selection because if user want to paste text to specific location,
> > user must tap to that location first and this action create a selection
> > change event with empty selection.
> 
> You can look at selectedText and compare it against an empty string for that
> purpose, right?
You mentioned below that we can use selectedText to indicate whether show the bubble. But in this case we need show the bubble even with empty selection (at least we have to show "paste" utility). If I use selectedText to this purpose, I cannot use it to indicate show the bubble or not. Do you have any idea about this?
> 
> > I have another scenario about bubble menu close. If user scrolling the view
> > or changing selection by using selection caret, the bubble should close
> > (described in spec page 4 figure 3 and page 17 figure 2). Right now I use a
> > dom event which called "TextualMenuClose" to indicate bubble close. You can
> > find it in my patch. I think we can integrate this event with this api by
> > add a attribute to SelectionChangeEvent, such as "isShowMenu"? Is it make
> > sense?
> 
> Hmm, let me see if I understand this correctly.  For the part about page 4
> figure 3, we're extending the selection by moving the selection handles,
> right?  In which case, I would expect the code would keep handling the
> selectionchange events that the content loaded inside mozbrowser would keep
> sending and keep the bubble visible as long as the selectedText is not
> empty.
Yes, we are extending it by moving the selection carets. But figure 3 says the bubble will disappear while dragging the handles. We cannot keep the bubble visible in this case. In my current patch, I use StartBatchChanges and EndBatchChanges [1] to prevent keep sending selection event when dragging the handles and I'll send a bubble close event when start dragging the handles.

[1] http://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFrameSelection.h#585

> For page 17 figure 2, you'd see that the selectedText is set to an
> empty string and you would hide the bubble.  Something like this on the
> embedder side:
Actually scroll the element doesn't clear the selection, so the selection event doesn't be fired. In my current gecko implementation, I'll also fire a bubble close event when scroll happened.
> 
>   var bubbleVisible = false;
>   function selectionChangeHandler(event) {
>     if (bubbleVisible && event.selectedText == "") {
>       hideBubble();
>       bubbleVisible = false;
>     } else if (!bubbleVisible && event.selectedText != "") {
>       showBubble();
>       bubbleVisible = true;
>     }
>   }
> 
> I think that should work.  What do you think?
As mentioned above, use selectedText to indicate whether show the bubble is conflict between 2 purposes. I think we need use other strategy here.
> 
> > > No, that was intended to go on mozbrowser iframes, as well as
> > > isCommandEnabled and doCommand below.
> > Sorry, I'm not very familiar with webidl stuff. Could you tell me mozbrowser
> > iframes belong to which webidl interface? Thanks.
> 
> mozbrowser is unfortunately not ported to WebIDL yet.  I was just using
> WebIDL as a language to explain the API.  The mozbrowser functions should
> probably be added like this:
> <http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/
> BrowserElementParent.jsm#152>
> 
> That being said, you should be able to define SelectionChangeEvent in
> WebIDL.  I think there is a way to generate an event class, please check
> with smaug on how that works.  Hopefully you won't have to write any C++
> code for the event class.
I got it. Thanks.

Comment 41

[(no longer active)]

(In reply to Morris Tseng [:mtseng] from comment #40)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #39)
> > (In reply to Morris Tseng [:mtseng] from comment #38)
> > > > Yes, you're right about the API.  We should keep the current UX spec and
> > > > ensure that the API will satisfy the UX spec, but I don't think that we need
> > > > to design the API in a way that can only satisfy the current UX spec,
> > > > because then the next time that the desired UX changes, we'd need to change
> > > > the API.
> > > > 
> > > > Based on that, my idea was to use the selectionchange event to make the
> > > > decision on when to show the bubble at the embedder side.  Basically, the
> > > > embedder will monitor the status of the selection in the mozbrowser hosted
> > > > content and will show a bubble when it detects that a new non-empty
> > > > selection was created because of a mouse-up event, and then it would hide
> > > > the bubble when it detects that the selection was changed to be empty again.
> > > > It could also use the selectionchange events' rect to move the bubble when
> > > > the selection moves.  This way, we can use the same API to implement other
> > > > possible UX specs in the future.
> > > > 
> > > > Do you think that makes sense?
> > > Yes, this makes sense. There is one thing that is we're also care about
> > > empty selection because if user want to paste text to specific location,
> > > user must tap to that location first and this action create a selection
> > > change event with empty selection.
> > 
> > You can look at selectedText and compare it against an empty string for that
> > purpose, right?
> You mentioned below that we can use selectedText to indicate whether show
> the bubble. But in this case we need show the bubble even with empty
> selection (at least we have to show "paste" utility). If I use selectedText
> to this purpose, I cannot use it to indicate show the bubble or not. Do you
> have any idea about this?

Hmm, what's the case where we want to show the paste bubble without any selection again?  If you want to do that for let's say when we focus an editable field even with a collapsed selection, then I'd say we would need to add a focus change event for that case (but I prefer to do that in another bug -- this one is getting lengthy!)

> > > I have another scenario about bubble menu close. If user scrolling the view
> > > or changing selection by using selection caret, the bubble should close
> > > (described in spec page 4 figure 3 and page 17 figure 2). Right now I use a
> > > dom event which called "TextualMenuClose" to indicate bubble close. You can
> > > find it in my patch. I think we can integrate this event with this api by
> > > add a attribute to SelectionChangeEvent, such as "isShowMenu"? Is it make
> > > sense?
> > 
> > Hmm, let me see if I understand this correctly.  For the part about page 4
> > figure 3, we're extending the selection by moving the selection handles,
> > right?  In which case, I would expect the code would keep handling the
> > selectionchange events that the content loaded inside mozbrowser would keep
> > sending and keep the bubble visible as long as the selectedText is not
> > empty.
> Yes, we are extending it by moving the selection carets. But figure 3 says
> the bubble will disappear while dragging the handles. We cannot keep the
> bubble visible in this case. In my current patch, I use StartBatchChanges
> and EndBatchChanges [1] to prevent keep sending selection event when
> dragging the handles and I'll send a bubble close event when start dragging
> the handles.

I think that's the wrong thing to do, because the selection is actually changing.  Note that other people might use the selectionchanged event for other purposes so I'd like to avoid tying it to the UX spec like that.  Basically the API should make it possible to implement the UX spec, but we should keep it as generic as we can so that different user interactions can also be built on top of it.

We could dispatch a different event when the handles are being dragged (again we should do that in a different bug) if needed.

> > For page 17 figure 2, you'd see that the selectedText is set to an
> > empty string and you would hide the bubble.  Something like this on the
> > embedder side:
> Actually scroll the element doesn't clear the selection, so the selection
> event doesn't be fired. In my current gecko implementation, I'll also fire a
> bubble close event when scroll happened.

It depends on how you scroll, right?  I would expect the selection to get cleared when you tap somewhere to begin scrolling, but I could be wrong.  The UX spec is way too vague about the details here.  But we can add events to reflect whether we're scrolling if needed.

> >   var bubbleVisible = false;
> >   function selectionChangeHandler(event) {
> >     if (bubbleVisible && event.selectedText == "") {
> >       hideBubble();
> >       bubbleVisible = false;
> >     } else if (!bubbleVisible && event.selectedText != "") {
> >       showBubble();
> >       bubbleVisible = true;
> >     }
> >   }
> > 
> > I think that should work.  What do you think?
> As mentioned above, use selectedText to indicate whether show the bubble is
> conflict between 2 purposes. I think we need use other strategy here.

Hopefully the above suggestions help?  :-)  Please let me know what you think.  Thanks!

Comment 42

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

All the suggestions help a lot. I agree that we focus on generic selection change event in this bug and fix other special case in another bug. I'll submit a WIP patch and ask for feedback asap. Thanks a lot.

Comment 43

[(no longer active)]

Thank you Morris!

Comment 44

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: (WIP) add-browserapi v11

Update patch base on comment.

Ehsan, right now UpdateCommands fired only when collapsed state is changed which does not match our requirement. So I add another code to call UpdateCommand with string "selectionchange". You can check the patch with this change.

Comment 45

[(no longer active)]

Comment on attachment 8434860 [details] [diff] [review]
(WIP) add-browserapi v11

Review of attachment 8434860 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Morris, I think you're generally right on track.  There are some issues that I noticed in your patch and a couple of places where I wasn't really sure why you're doing things that way, but it mostly looks good.

::: b2g/chrome/content/shell.js
@@ +519,5 @@
> +          type: 'textualmenu',
> +          detail: evt.detail
> +        });
> +        evt.stopPropagation();
> +        evt.preventDefault();

I don't think I understand this part of the setup either.

::: dom/base/nsGlobalWindow.cpp
@@ +9198,5 @@
>      nsCOMPtr<nsIDOMXULCommandDispatcher> xulCommandDispatcher;
>      xulDoc->GetCommandDispatcher(getter_AddRefs(xulCommandDispatcher));
>      if (xulCommandDispatcher) {
>        nsContentUtils::AddScriptRunner(new CommandDispatcher(xulCommandDispatcher,
>                                                              anAction));

I think you need to special case this code to make it not run for "selectionchange", because that would basically expose a new event to XUL apps, which is not necessarily desirable here.

@@ +9220,5 @@
> +        SelectionChangeEvent::Constructor(mDoc, NS_LITERAL_STRING("selectionchange"), init);
> +
> +      event->SetTrusted(true);
> +      bool ret;
> +      mDoc->DispatchEvent(event, &ret);

I think this will expose the event to normal web pages, which is not something that we want.  Please check with Olli on how we can avoid doing that for events, I'm not 100% sure.

::: dom/bindings/Bindings.conf
@@ +1980,5 @@
>                   headerFile='nsIAsyncInputStream.h')
>  addExternalIface('nsIMessageBroadcaster', nativeType='nsIMessageBroadcaster',
>                   headerFile='nsIMessageManager.h', notflattened=True)
>  addExternalIface('nsISelectionListener', nativeType='nsISelectionListener')
> +addExternalIface('nsISelection', nativeType='nsISelection')

I think you can avoid doing this by using Selection.

::: dom/browser-element/BrowserElementChildPreload.js
@@ +618,5 @@
> +      detail.right += nowRect.left;
> +      nowWindow = nowWindow.parent;
> +    }
> +
> +    sendAsyncMsg("textualmenu", detail);

Why are you still sending "textualmenu"?  :-)

::: dom/browser-element/BrowserElementParent.jsm
@@ +493,5 @@
> +    let evt = this._createEvent('textualmenu', detail, true);
> +    let self = this;
> +    defineAndExpose(evt.detail, 'doCommand', function(cmd) {
> +      self._sendAsyncMsg('do-command', {command: cmd});
> +    });

I don't really understand this part of your setup here.  I think you're defining the doCommand function lazily.  I don't understand why.

Also, I would like to completely move away from the "textualmenu" event.

::: dom/interfaces/base/nsIDOMWindow.idl
@@ +424,5 @@
>                                         in DOMString options,
>                                         in nsISupports aExtraArgument);
>  
>    // XXX Should this be in nsIDOMChromeWindow?
> +  void                      updateCommands(in DOMString action,

You need to rev the uuid of this interface.

::: dom/webidl/SelectionChangeEvent.webidl
@@ +1,1 @@
> +dictionary SelectionChangeEventInit : EventInit {

Nit: please add the license header on all of the files you add.

@@ +4,5 @@
> +  short? reason = 0;
> +};
> +
> +[Constructor(DOMString type, optional SelectionChangeEventInit eventInit)]
> +interface SelectionChangeEvent : Event {

Again, this will expose the interface to normal web pages which is undesirable here.

::: dom/webidl/Window.webidl
@@ +302,5 @@
>    [Throws, ChromeOnly] void             home();
>  
>    // XXX Should this be in nsIDOMChromeWindow?
> +  void                      updateCommands(DOMString action,
> +                                           optional nsISelection? sel = null,

Nit: couldn't you use Selection here?

Comment 46

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #45)
> 
> @@ +9220,5 @@
> > +        SelectionChangeEvent::Constructor(mDoc, NS_LITERAL_STRING("selectionchange"), init);
> > +
> > +      event->SetTrusted(true);
> > +      bool ret;
> > +      mDoc->DispatchEvent(event, &ret);
> 
> I think this will expose the event to normal web pages, which is not
> something that we want.  Please check with Olli on how we can avoid doing
> that for events, I'm not 100% sure.
Hi Olli,
We want to dispatch a event which won't expose to normal web pages. This event only dispatch to BrowserElementChild. How can I do this? Is it correct for using nsContentUtils::DispatchChromeEvent here? Do you have any suggestion? Thanks.

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

Yeah,  nsContentUtils::DispatchChromeEvent sounds right.

Comment 48

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Olli Pettay [:smaug] from comment #47)
> Yeah,  nsContentUtils::DispatchChromeEvent sounds right.

But I found that DispatchChromeEvent only passes event as a string. I need dispatch event by passing pointer of nsIDOMEvent as parameter. So, is it the same if I set that nsIDOMEvent to trusted event by calling SetTrusted(true) and dispatch it using mDoc->DispatchEvent()? Thanks.

Comment 49

[Olli Pettay [:smaug][[e-mail]]]

Ah, right.

You could also set mOnlyChromeDispatch flag.
event->GetInternalNSEvent()->mOnlyChromeDispatch = true;

Comment 50

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Implement mozbrowserselectionchange v1

Update to address comment.

Comment 51

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: (WIP)mochitest_for_copy_paste

Mochitest for this api

Comment 52

[(no longer active)]

Comment on attachment 8438102 [details] [diff] [review]
Implement mozbrowserselectionchange v1

Review of attachment 8438102 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on everything except for the shell.js and browser-element changes, I don't trust myself enough to review those parts.  Also, you didn't answer my questions about those bits in comment 45.  ;-)

::: dom/base/nsGlobalWindow.cpp
@@ +9187,5 @@
>      do_QueryInterface(rootWindow->GetExtantDoc());
>    // See if we contain a XUL document.
> +  // selectionchange action only use for mozbrowser, not for XUL. So we bypass
> +  // XUL command dispatch if it is selectionchange action.
> +  if (xulDoc && !anAction.Equals(NS_LITERAL_STRING("selectionchange"))) {

Nit: EqualsLiteral, and drop NS_LITERAL_STRING please.

Comment 53

[(no longer active)]

Comment on attachment 8438248 [details] [diff] [review]
(WIP)mochitest_for_copy_paste

Review of attachment 8438248 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good generally, but please test all of the following cases separately:

<input type=text>
<input type=password>
<input type=number>
<textarea>
<div contenteditable>
designMode
a normal document

Note that some commands such as cut and paste won't make sense for the last case obviously.

::: dom/browser-element/mochitest/browserElement_CopyPaste.js
@@ +27,5 @@
> +      width = e.detail.width;
> +      height = e.detail.height;
> +      iframe.doCommand("copy");
> +      iframe.doCommand("paste");
> +      iframe.doCommand("selectall");

It would be nice to test the effects of each one of these commands separately.

@@ +38,5 @@
> +      // paste twice
> +      iframe.doCommand("cut");
> +      iframe.doCommand("paste");
> +      iframe.doCommand("paste");
> +      iframe.doCommand("selectall");

Same here.

Comment 54

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #52)
> Comment on attachment 8438102 [details] [diff] [review]
> Implement mozbrowserselectionchange v1
> 
> Review of attachment 8438102 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me on everything except for the shell.js and browser-element changes, I
> don't trust myself enough to review those parts.  Also, you didn't answer my
> questions about those bits in comment 45.  ;-)
Thanks for review, Ehsan. I'll answer the question in next comment. Sorry about it.

> 
> ::: dom/base/nsGlobalWindow.cpp
> @@ +9187,5 @@
> >      do_QueryInterface(rootWindow->GetExtantDoc());
> >    // See if we contain a XUL document.
> > +  // selectionchange action only use for mozbrowser, not for XUL. So we bypass
> > +  // XUL command dispatch if it is selectionchange action.
> > +  if (xulDoc && !anAction.Equals(NS_LITERAL_STRING("selectionchange"))) {
> 
> Nit: EqualsLiteral, and drop NS_LITERAL_STRING please.
Ok, updated in next patch.

Comment 55

[(no longer active)]

(In reply to comment #54)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #52)
> > Comment on attachment 8438102 [details] [diff] [review]
> > Implement mozbrowserselectionchange v1
> > 
> > Review of attachment 8438102 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me on everything except for the shell.js and browser-element changes, I
> > don't trust myself enough to review those parts.  Also, you didn't answer my
> > questions about those bits in comment 45.  ;-)
> Thanks for review, Ehsan. I'll answer the question in next comment. Sorry about
> it.

No worries!  They will be mostly relevant to the next reviewer anyway!  :-)

Comment 56

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #45)
> Comment on attachment 8434860 [details] [diff] [review]
> (WIP) add-browserapi v11
> 
> Review of attachment 8434860 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks Morris, I think you're generally right on track.  There are some
> issues that I noticed in your patch and a couple of places where I wasn't
> really sure why you're doing things that way, but it mostly looks good.
> 
> ::: b2g/chrome/content/shell.js
> @@ +519,5 @@
> > +          type: 'textualmenu',
> > +          detail: evt.detail
> > +        });
> > +        evt.stopPropagation();
> > +        evt.preventDefault();
> 
> I don't think I understand this part of the setup either.
hmm, this is my test code. Remove it in latest patch.
> 
> 
> ::: dom/browser-element/BrowserElementChildPreload.js
> @@ +618,5 @@
> > +      detail.right += nowRect.left;
> > +      nowWindow = nowWindow.parent;
> > +    }
> > +
> > +    sendAsyncMsg("textualmenu", detail);
> 
> Why are you still sending "textualmenu"?  :-)
Changed it to "selectionchange" already.

> 
> ::: dom/browser-element/BrowserElementParent.jsm
> @@ +493,5 @@
> > +    let evt = this._createEvent('textualmenu', detail, true);
> > +    let self = this;
> > +    defineAndExpose(evt.detail, 'doCommand', function(cmd) {
> > +      self._sendAsyncMsg('do-command', {command: cmd});
> > +    });
> 
> I don't really understand this part of your setup here.  I think you're
> defining the doCommand function lazily.  I don't understand why.
In the latest patch, I do not use this way to expose function. I use defineNoReturnMethod as you suggested.
> 
> Also, I would like to completely move away from the "textualmenu" event.
Changed to "selectionchange" as well.

Comment 57

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Implement mozbrowserselectionchange v2 (carry r+: ehsan)

Hi Vivien,
Could you review shell.js and browser-element change in this patch? Thanks.

Comment 58

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: mochitest for copy paste v1

Handle textarea, input:text, input:number, input:password, contenteditable, normal element and document with designMode case.

I found some weird cases that when I doCommand with contenteditable element in OOP case and the command is not working. So I disable those cases now. I'll file another bug to track this issue.

Comment 59

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8439673 [details] [diff] [review]
Implement mozbrowserselectionchange v2 (carry r+: ehsan)

Review of attachment 8439673 [details] [diff] [review]:
-----------------------------------------------------------------

I would also like to see mozbrowser tests for this new API. I know you already did some tests but I expect those to be more oriented about checking the fact that doCommand works correctly. While I want to ensure that the messaging system works correctly, with correct geometry informations here.

Overall, it looks good but I would like to see a new version of the patch before r+'ing.

::: b2g/chrome/content/shell.js
@@ +343,5 @@
>      window.addEventListener('MozAfterPaint', this);
>      window.addEventListener('sizemodechange', this);
>      window.addEventListener('unload', this);
>      this.contentBrowser.addEventListener('mozbrowserloadstart', this, true);
> +    this.contentBrowser.addEventListener('mozbrowserselectionchange', this, false);

nit: no need for |false| here, this is the default.

@@ +370,5 @@
>      window.removeEventListener('MozApplicationManifest', this);
>      window.removeEventListener('mozfullscreenchange', this);
>      window.removeEventListener('sizemodechange', this);
>      this.contentBrowser.removeEventListener('mozbrowserloadstart', this, true);
> +    this.contentBrowser.removeEventListener('mozbrowserselectionchange', this, false);

nit: same comment as above.

@@ +740,2 @@
>      }
>    }

Can you explain a bit the reasoning behind this DoCommandHelper ?

My guess is:

It is done this way, so all the iframe.doCommand are done by shell.js in order to have selection working on all apps (system app and the one embedded by it).

So Gaia will send a do-command event as a response to the user action on the helper bubble, or based on some mozbrowser events ?

If yes, can't we instead send the target of the event into the mozContentEvent ? The target beeing an <iframe mozbrowser> of a content window ? (for the system app main window / inner non-mozbrowser iframes).

::: dom/browser-element/BrowserElementChildPreload.js
@@ +110,5 @@
> +  "cut": "cmd_cut",
> +  "copy": "cmd_copy",
> +  "paste": "cmd_paste",
> +  "selectall": "cmd_selectAll"
> +};

nit: for all, replace " by '

@@ +355,5 @@
> +    if (cmd in COMMAND_MAP) {
> +      return docShell.isCommandEnabled(COMMAND_MAP[cmd]);
> +    }
> +
> +    return false;

nit: I would write the function the other way personaly.

let command = COMMAND_MAP[cmd];
if (!command) {
  return false;
}

return docShell.isCommandEnabled(command);

@@ +359,5 @@
> +    return false;
> +  },
> +
> +  _doCommand: function(cmd) {
> +    if (cmd in COMMAND_MAP) {

Maybe you want to use this._isCommandEnabled(cmd) here ?

@@ +588,5 @@
>  
> +  _selectionChangeHandler: function(e) {
> +    let isMouseUp = e.reason & Ci.nsISelectionListener.MOUSEUP_REASON;
> +    let isSelectAll = e.reason & Ci.nsISelectionListener.SELECTALL_REASON;
> +    if (!(isMouseUp || (isSelectAll && e.selectedText.length > 0))) {

Just curious. Why do we need to look for the lenght of the selected text here ?

Is is in case the user unselect everything ?

@@ +590,5 @@
> +    let isMouseUp = e.reason & Ci.nsISelectionListener.MOUSEUP_REASON;
> +    let isSelectAll = e.reason & Ci.nsISelectionListener.SELECTALL_REASON;
> +    if (!(isMouseUp || (isSelectAll && e.selectedText.length > 0))) {
> +      return;
> +    }

nit: add a line break after a return block.

@@ +604,5 @@
> +                   canSelectAll: this._isCommandEnabled("selectall"),
> +                   canCut: this._isCommandEnabled("cut"),
> +                   canCopy: this._isCommandEnabled("copy"),
> +                   canPaste: this._isCommandEnabled("paste"),
> +                   show: true,

What is |show: true|, used for ? Seems like it can be omitted if it is always |true|.

@@ +605,5 @@
> +                   canCut: this._isCommandEnabled("cut"),
> +                   canCopy: this._isCommandEnabled("copy"),
> +                   canPaste: this._isCommandEnabled("paste"),
> +                   show: true,
> +                   zoomFactor: zoomFactor};

nit:

let detail = {
 ...
};

Do we want |detail| to be a bit more organized too ?

like
let detail = {
  rect: boundingClientRect,
  zoomFactor: zoomFactor,
  commands: {
    canSelectAll: ...
    ....
  }
}; 

?

@@ +609,5 @@
> +                   zoomFactor: zoomFactor};
> +
> +    // If we're not top window, we need shift position relative
> +    // to parent window
> +    let nowWindow = e.target.defaultView;

nit: s/nowWindow/currentWindow

@@ +617,5 @@
> +      detail.bottom += nowRect.top;
> +      detail.left += nowRect.left;
> +      detail.right += nowRect.left;
> +      nowWindow = nowWindow.parent;
> +    }

I didn't looked at all the platform code so it's a bit fuzzy to me and I may be wrong but:

In the case of an OOP <iframe mozbrowser> embedded in the system app, and if this iframe embed itself a In-Process <iframe mozbrowser>, does the selection change event is forwarded to the system app only if the OOP <iframe> does not handle it ?

If yes, I think we also need to cross the <iframe mozbrowser> boundary to have the correct geometry information here. So we need to use |realFrameElement| as well.

@@ +620,5 @@
> +      nowWindow = nowWindow.parent;
> +    }
> +
> +    sendAsyncMsg("selectionchange", detail);
> +    e.stopPropagation();

nit: I usually like to call this kind of thing early in the method (like right after the first return block). So it is clear that the event won't propagate as soon as you start to read the method.

@@ +1057,5 @@
>    },
>  
> +  _recvDoCommand: function(data) {
> +    this._doCommand(data.json.command);
> +  },

Do we really need a separate _doCommand method for that ? What about moving the doCommand code inside this method ?

::: dom/browser-element/BrowserElementParent.jsm
@@ +134,5 @@
>    defineNoReturnMethod('goBack', this._goBack);
>    defineNoReturnMethod('goForward', this._goForward);
>    defineNoReturnMethod('reload', this._reload);
>    defineNoReturnMethod('stop', this._stop);
> +  defineNoRetuenMethod('doCommand', this._doCommand);

s/defineNoRetuenMethod/defineNoReturnMethod

@@ +467,5 @@
>      }
>    },
>  
> +  _handleSelectionChange: function(data) {
> +    let detail = data.json;

Since |detail| is only used once in this method, you don't need to make it a local var. Used directly |data.json| in createEvent.

@@ +478,5 @@
> +    let rect = this._frameElement.getBoundingClientRect();
> +    offsetX += rect.left;
> +    offsetY += rect.top;
> +    detail.frameOffsetX = offsetX;
> +    detail.frameOffsetY = offsetY;

Why do you need those geometry informations here ? Can't those be retrieved by the embedder or did I missed something ?

Comment 60

[(no longer active)]

Comment on attachment 8441258 [details] [diff] [review]
mochitest for copy paste v1

Review of attachment 8441258 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly minusing because of waitForClipboard which can make parts of the code here async so I'd like to look at the resulting patch again.  But I'd also like to understand the dummy wait stuff.

(Also note that we should investigate and fix the existing failures before we ship this, but of course it doesn't need to block landing this as long you file the follow-up!)

::: dom/browser-element/mochitest/browserElement_CopyPaste.js
@@ +19,5 @@
> +  gTextarea.value = "";
> +  SpecialPowers.wrap(gTextarea).editor.paste(SpecialPowers.Ci.nsIClipboard.kGlobalClipboard);
> +  var clipboardData = gTextarea.value;
> +  return clipboardData;
> +}

Instead of using a textarea for this, why not use SimpleTest.waitForClipboard here?

@@ +173,5 @@
> +    mm.removeMessageListener('dummy-wait', messageforcopy);
> +    testCopy2(e);
> +  });
> +
> +  mm.loadFrameScript(getScriptForDummyWait(), false);

Why do you need to do this dummy waiting?

::: dom/browser-element/mochitest/file_browserElement_CopyPaste.html
@@ +1,3 @@
> +<html>
> +<body>
> +  <textarea id="text">Test for selection change event</textarea>

To avoid confusion, I think it would be slightly better to make this an empty document.

Comment 61

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #59)
> Comment on attachment 8439673 [details] [diff] [review]
> Implement mozbrowserselectionchange v2 (carry r+: ehsan)
> 
> Review of attachment 8439673 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would also like to see mozbrowser tests for this new API. I know you
> already did some tests but I expect those to be more oriented about checking
> the fact that doCommand works correctly. While I want to ensure that the
> messaging system works correctly, with correct geometry informations here.
I think my test checks doCommand and message system works correctly. I'll add more test to check correct geometry information.
> 
> Overall, it looks good but I would like to see a new version of the patch
> before r+'ing.
> 
> ::: b2g/chrome/content/shell.js
> @@ +343,5 @@
> >      window.addEventListener('MozAfterPaint', this);
> >      window.addEventListener('sizemodechange', this);
> >      window.addEventListener('unload', this);
> >      this.contentBrowser.addEventListener('mozbrowserloadstart', this, true);
> > +    this.contentBrowser.addEventListener('mozbrowserselectionchange', this, false);
> 
> nit: no need for |false| here, this is the default.
done
> 
> @@ +370,5 @@
> >      window.removeEventListener('MozApplicationManifest', this);
> >      window.removeEventListener('mozfullscreenchange', this);
> >      window.removeEventListener('sizemodechange', this);
> >      this.contentBrowser.removeEventListener('mozbrowserloadstart', this, true);
> > +    this.contentBrowser.removeEventListener('mozbrowserselectionchange', this, false);
> 
> nit: same comment as above.
done
> 
> @@ +740,2 @@
> >      }
> >    }
> 
> Can you explain a bit the reasoning behind this DoCommandHelper ?
> 
> My guess is:
> 
> It is done this way, so all the iframe.doCommand are done by shell.js in
> order to have selection working on all apps (system app and the one embedded
> by it).
Not all doCommand are done by shell.js. shell.js only handle system app, inner non-mozbrowser iframes and mozbrowser iframes which is not managed by gaia's window manager such as each tab of browser app.

In normal apps, they can just call doCommand through their iframe. So this is easy case.
In other cases which are handled by shell.js, we cache latest event in DoCommandHelper and wait for mozContentEvent with type do-command. Once we get the do-command, retrive iframe from event.target and we could call DoCommand from this ifrmae.
> 
> So Gaia will send a do-command event as a response to the user action on the
> helper bubble, or based on some mozbrowser events ?
For the cases handled by shell.js, yes, they send a do-command event. For normal apps, we just call DoCommand by its iframe.

Some code snippet is here [1] from bug 1024882

[1]: https://github.com/cctuan/gaia/commit/4fe193cf6cdddf9f8c3b8917e9dce1f90c61d527#diff-a9dd9b124326536dcec4a14abf918f8dR112
> 
> If yes, can't we instead send the target of the event into the
> mozContentEvent ? The target beeing an <iframe mozbrowser> of a content
> window ? (for the system app main window / inner non-mozbrowser iframes).
I tried reverse way which is sending target into the mozChromeEvent. But once I put target info into detail, I get NS_ERROR_FAILURE in cloneInto.

I cannot send the target of the event into the mozContentEvent as well because I cannot tell which iframe is belong to in the embedder side.

I think we can get rid of DoCommandHelper once we can pass target info by mozChromeEvent/mozContentEvent. Do you have any idea how we can do this?
> 
> ::: dom/browser-element/BrowserElementChildPreload.js
> @@ +110,5 @@
> > +  "cut": "cmd_cut",
> > +  "copy": "cmd_copy",
> > +  "paste": "cmd_paste",
> > +  "selectall": "cmd_selectAll"
> > +};
> 
> nit: for all, replace " by '
done
> 
> @@ +355,5 @@
> > +    if (cmd in COMMAND_MAP) {
> > +      return docShell.isCommandEnabled(COMMAND_MAP[cmd]);
> > +    }
> > +
> > +    return false;
> 
> nit: I would write the function the other way personaly.
> 
> let command = COMMAND_MAP[cmd];
> if (!command) {
>   return false;
> }
> 
> return docShell.isCommandEnabled(command);
done
> 
> @@ +359,5 @@
> > +    return false;
> > +  },
> > +
> > +  _doCommand: function(cmd) {
> > +    if (cmd in COMMAND_MAP) {
> 
> Maybe you want to use this._isCommandEnabled(cmd) here ?
done
> 
> @@ +588,5 @@
> >  
> > +  _selectionChangeHandler: function(e) {
> > +    let isMouseUp = e.reason & Ci.nsISelectionListener.MOUSEUP_REASON;
> > +    let isSelectAll = e.reason & Ci.nsISelectionListener.SELECTALL_REASON;
> > +    if (!(isMouseUp || (isSelectAll && e.selectedText.length > 0))) {
> 
> Just curious. Why do we need to look for the lenght of the selected text
> here ?
> 
> Is is in case the user unselect everything ?
In gecko, selectall will first collapse range then select all element [2]. Both actions will send selection change event with SELECTALL reason. We want to filter first collapse event so I check the length of selected text.

[2]: http://dxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#5177
> 
> @@ +590,5 @@
> > +    let isMouseUp = e.reason & Ci.nsISelectionListener.MOUSEUP_REASON;
> > +    let isSelectAll = e.reason & Ci.nsISelectionListener.SELECTALL_REASON;
> > +    if (!(isMouseUp || (isSelectAll && e.selectedText.length > 0))) {
> > +      return;
> > +    }
> 
> nit: add a line break after a return block.
done
> 
> @@ +604,5 @@
> > +                   canSelectAll: this._isCommandEnabled("selectall"),
> > +                   canCut: this._isCommandEnabled("cut"),
> > +                   canCopy: this._isCommandEnabled("copy"),
> > +                   canPaste: this._isCommandEnabled("paste"),
> > +                   show: true,
> 
> What is |show: true|, used for ? Seems like it can be omitted if it is
> always |true|.
We have another case to tell embedder close the bubble which will send this event with |show: false|. But that case does not address in this bug.
> 
> @@ +605,5 @@
> > +                   canCut: this._isCommandEnabled("cut"),
> > +                   canCopy: this._isCommandEnabled("copy"),
> > +                   canPaste: this._isCommandEnabled("paste"),
> > +                   show: true,
> > +                   zoomFactor: zoomFactor};
> 
> nit:
> 
> let detail = {
>  ...
> };
> 
> Do we want |detail| to be a bit more organized too ?
> 
> like
> let detail = {
>   rect: boundingClientRect,
>   zoomFactor: zoomFactor,
>   commands: {
>     canSelectAll: ...
>     ....
>   }
> }; 
> 
> ?
done
> 
> @@ +609,5 @@
> > +                   zoomFactor: zoomFactor};
> > +
> > +    // If we're not top window, we need shift position relative
> > +    // to parent window
> > +    let nowWindow = e.target.defaultView;
> 
> nit: s/nowWindow/currentWindow
done
> 
> @@ +617,5 @@
> > +      detail.bottom += nowRect.top;
> > +      detail.left += nowRect.left;
> > +      detail.right += nowRect.left;
> > +      nowWindow = nowWindow.parent;
> > +    }
> 
> I didn't looked at all the platform code so it's a bit fuzzy to me and I may
> be wrong but:
> 
> In the case of an OOP <iframe mozbrowser> embedded in the system app, and if
> this iframe embed itself a In-Process <iframe mozbrowser>, does the
> selection change event is forwarded to the system app only if the OOP
> <iframe> does not handle it ?
> 
> If yes, I think we also need to cross the <iframe mozbrowser> boundary to
> have the correct geometry information here. So we need to use
> |realFrameElement| as well.
done
> 
> @@ +620,5 @@
> > +      nowWindow = nowWindow.parent;
> > +    }
> > +
> > +    sendAsyncMsg("selectionchange", detail);
> > +    e.stopPropagation();
> 
> nit: I usually like to call this kind of thing early in the method (like
> right after the first return block). So it is clear that the event won't
> propagate as soon as you start to read the method.
done
> 
> @@ +1057,5 @@
> >    },
> >  
> > +  _recvDoCommand: function(data) {
> > +    this._doCommand(data.json.command);
> > +  },
> 
> Do we really need a separate _doCommand method for that ? What about moving
> the doCommand code inside this method ?
done
> 
> ::: dom/browser-element/BrowserElementParent.jsm
> @@ +134,5 @@
> >    defineNoReturnMethod('goBack', this._goBack);
> >    defineNoReturnMethod('goForward', this._goForward);
> >    defineNoReturnMethod('reload', this._reload);
> >    defineNoReturnMethod('stop', this._stop);
> > +  defineNoRetuenMethod('doCommand', this._doCommand);
> 
> s/defineNoRetuenMethod/defineNoReturnMethod
done
> 
> @@ +467,5 @@
> >      }
> >    },
> >  
> > +  _handleSelectionChange: function(data) {
> > +    let detail = data.json;
> 
> Since |detail| is only used once in this method, you don't need to make it a
> local var. Used directly |data.json| in createEvent.
done
> 
> @@ +478,5 @@
> > +    let rect = this._frameElement.getBoundingClientRect();
> > +    offsetX += rect.left;
> > +    offsetY += rect.top;
> > +    detail.frameOffsetX = offsetX;
> > +    detail.frameOffsetY = offsetY;
> 
> Why do you need those geometry informations here ? Can't those be retrieved
> by the embedder or did I missed something ?
Those info only use by event which dispatch to shell.js. This case maybe across chrome <-> content boundary. realFrameElement does not handle chrome-content boundary so I add this code to handle this. To avoid confusion, I move this code to shell.js instead of BrowserElementParent.jsm.

Comment 62

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Implement mozbrowserselectionchange v3 (carry r+: ehsan)

Update to address comment

Comment 63

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #60)
> Comment on attachment 8441258 [details] [diff] [review]
> mochitest for copy paste v1
> 
> Review of attachment 8441258 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mostly minusing because of waitForClipboard which can make parts of the code
> here async so I'd like to look at the resulting patch again.  But I'd also
> like to understand the dummy wait stuff.
Initial idea for dummy wait is wait a period of time by this ping-pong event so that child side have enough time to complete the command. But since we have waitForClipboard function, we don't need this mechanism. So I remove it completely.
> 
> (Also note that we should investigate and fix the existing failures before
> we ship this, but of course it doesn't need to block landing this as long
> you file the follow-up!)
OK! thanks
> 
> ::: dom/browser-element/mochitest/browserElement_CopyPaste.js
> @@ +19,5 @@
> > +  gTextarea.value = "";
> > +  SpecialPowers.wrap(gTextarea).editor.paste(SpecialPowers.Ci.nsIClipboard.kGlobalClipboard);
> > +  var clipboardData = gTextarea.value;
> > +  return clipboardData;
> > +}
> 
> Instead of using a textarea for this, why not use
> SimpleTest.waitForClipboard here?
hmm, I didn't know waitForClipboard before. Thanks for the information. I have updated my patch to use waitForClipboard.
> 
> @@ +173,5 @@
> > +    mm.removeMessageListener('dummy-wait', messageforcopy);
> > +    testCopy2(e);
> > +  });
> > +
> > +  mm.loadFrameScript(getScriptForDummyWait(), false);
> 
> Why do you need to do this dummy waiting?
Removed.
> 
> ::: dom/browser-element/mochitest/file_browserElement_CopyPaste.html
> @@ +1,3 @@
> > +<html>
> > +<body>
> > +  <textarea id="text">Test for selection change event</textarea>
> 
> To avoid confusion, I think it would be slightly better to make this an
> empty document.
This file is not used anymore. Removed it in next patch.

Comment 64

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: mochitest for copy paste v2

Update to address comment.

Comment 65

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Morris Tseng [:mtseng] from comment #61)
> > 
> > Can you explain a bit the reasoning behind this DoCommandHelper ?
> > 
> > My guess is:
> > 
> > It is done this way, so all the iframe.doCommand are done by shell.js in
> > order to have selection working on all apps (system app and the one embedded
> > by it).
> Not all doCommand are done by shell.js. shell.js only handle system app,
> inner non-mozbrowser iframes and mozbrowser iframes which is not managed by
> gaia's window manager such as each tab of browser app.
> 
> In normal apps, they can just call doCommand through their iframe. So this
> is easy case.
> In other cases which are handled by shell.js, we cache latest event in
> DoCommandHelper and wait for mozContentEvent with type do-command. Once we
> get the do-command, retrive iframe from event.target and we could call
> DoCommand from this ifrmae.
> > 
> > So Gaia will send a do-command event as a response to the user action on the
> > helper bubble, or based on some mozbrowser events ?
> For the cases handled by shell.js, yes, they send a do-command event. For
> normal apps, we just call DoCommand by its iframe.
> 
> Some code snippet is here [1] from bug 1024882
> 
> [1]:
> https://github.com/cctuan/gaia/commit/
> 4fe193cf6cdddf9f8c3b8917e9dce1f90c61d527#diff-
> a9dd9b124326536dcec4a14abf918f8dR112
> > 
> > If yes, can't we instead send the target of the event into the
> > mozContentEvent ? The target beeing an <iframe mozbrowser> of a content
> > window ? (for the system app main window / inner non-mozbrowser iframes).
> I tried reverse way which is sending target into the mozChromeEvent. But
> once I put target info into detail, I get NS_ERROR_FAILURE in cloneInto.
> 
> I cannot send the target of the event into the mozContentEvent as well
> because I cannot tell which iframe is belong to in the embedder side.
> 
> I think we can get rid of DoCommandHelper once we can pass target info by
> mozChromeEvent/mozContentEvent. Do you have any idea how we can do this?

I think what you can do here is to call <targetIframe>.dispatchEvent('mozContentEvent', ...) and retrieve the event target in shell.js by loooking at event.target. Or dispatch the event on the system app window, for the system app case.

Basically dispatching the event on the right target is a way to workaround the limitation of cloneInto. We do that in devtools.js and the PermissionPromptHelper.


> > @@ +588,5 @@
> > >  
> > > +  _selectionChangeHandler: function(e) {
> > > +    let isMouseUp = e.reason & Ci.nsISelectionListener.MOUSEUP_REASON;
> > > +    let isSelectAll = e.reason & Ci.nsISelectionListener.SELECTALL_REASON;
> > > +    if (!(isMouseUp || (isSelectAll && e.selectedText.length > 0))) {
> > 
> > Just curious. Why do we need to look for the lenght of the selected text
> > here ?
> > 
> > Is is in case the user unselect everything ?
> In gecko, selectall will first collapse range then select all element [2].
> Both actions will send selection change event with SELECTALL reason. We want
> to filter first collapse event so I check the length of selected text.
>

Makes sense. Can you add a comment to the code explaining this ?
 
> > 
> > @@ +604,5 @@
> > > +                   canSelectAll: this._isCommandEnabled("selectall"),
> > > +                   canCut: this._isCommandEnabled("cut"),
> > > +                   canCopy: this._isCommandEnabled("copy"),
> > > +                   canPaste: this._isCommandEnabled("paste"),
> > > +                   show: true,
> > 
> > What is |show: true|, used for ? Seems like it can be omitted if it is
> > always |true|.
> We have another case to tell embedder close the bubble which will send this
> event with |show: false|. But that case does not address in this bug.
> >

Let's remove |show| from this patch then. And you will add it into the other bug.
 


Also, what happens if the user does a zoom gesture while a selection is active ? Is there a separate issue for that ? (I believe this is related to the |show| flag).

Comment 66

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8441930 [details] [diff] [review]
Implement mozbrowserselectionchange v3 (carry r+: ehsan)

Review of attachment 8441930 [details] [diff] [review]:
-----------------------------------------------------------------

Let's see if we can get rid of DoCommandHelper by dispatching mozChromeEvent/mozContentEvent directly on the right iframe.

::: dom/browser-element/BrowserElementChildPreload.js
@@ +585,5 @@
> +    let isMouseUp = e.reason & Ci.nsISelectionListener.MOUSEUP_REASON;
> +    let isSelectAll = e.reason & Ci.nsISelectionListener.SELECTALL_REASON;
> +    if (!(isMouseUp || (isSelectAll && e.selectedText.length > 0))) {
> +      return;
> +    }

As said in my previous comment, let's add a comment that explain why you are checking the length of the selected text.

@@ +599,5 @@
> +        top: boundingClientRect.top,
> +        bottom: boundingClientRect.bottom,
> +        left: boundingClientRect.left,
> +        right: boundingClientRect.right,
> +      },

Can't you just |rect: boundingClientRect| ?

@@ +607,5 @@
> +        canCopy: this._isCommandEnabled("copy"),
> +        canPaste: this._isCommandEnabled("paste"),
> +      },
> +      zoomFactor: zoomFactor,
> +      show: true

Let's remove |show| for now, and add it into the related bug.

::: dom/browser-element/BrowserElementParent.jsm
@@ +467,5 @@
>      }
>    },
>  
> +  _handleSelectionChange: function(data) {
> +    let evt = this._createEvent('selectionchange', data.json, true);

Are you sure that the event is cancellable (i feel like it is not) ? (also add the /* cancellable */ comment like the rest of the code.

Comment 67

[(no longer active)]

Comment on attachment 8441964 [details] [diff] [review]
mochitest for copy paste v2

Review of attachment 8441964 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

::: dom/browser-element/mochitest/browserElement_CopyPaste.js
@@ +163,5 @@
> +    ok(success, "copy command works" + stateMeaning);
> +    SimpleTest.executeSoon(function() { testPaste1(e); });
> +  };
> +
> +  let success = function() {

FWIW the following construct would also define a variable with name |success| which points to this function:

function success() {...}

Comment 68

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #65)
> I think what you can do here is to call
> <targetIframe>.dispatchEvent('mozContentEvent', ...) and retrieve the event
> target in shell.js by loooking at event.target. Or dispatch the event on the
> system app window, for the system app case.
> 
> Basically dispatching the event on the right target is a way to workaround
> the limitation of cloneInto. We do that in devtools.js and the
> PermissionPromptHelper.
> 
I have trouble with how to get <targetIframe>. For example, I use browser app to navigate a website, so the dom hierarchy look like this:

<shell.html>
  <systemapp> (mozbrowser, mozapp)
    <browserapp> (mozbrowser, mozapp)
      <tab> (mozbrowser, remote)

When selection change event occurred, BrowserElementParent of <tab> dispatch 'mozbrowserselectionchange' event to its _frameElement. And shell.js receives this event then shell.js dispatch 'mozChromeEvent' to systemapp and the bubble shows. When I click on bubbles, systemapp should send 'mozContentEvent' to <targetIFrame>. In this case, correct <targetIFrame> is <tab>. But how can I get <tab> frame in the systemapp? I'm stuck with this problem, can you give some suggestion? Thanks.

Comment 69

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #66)
> Comment on attachment 8441930 [details] [diff] [review]
> Implement mozbrowserselectionchange v3 (carry r+: ehsan)
> 
> Review of attachment 8441930 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +599,5 @@
> > +        top: boundingClientRect.top,
> > +        bottom: boundingClientRect.bottom,
> > +        left: boundingClientRect.left,
> > +        right: boundingClientRect.right,
> > +      },
> 
> Can't you just |rect: boundingClientRect| ?
Because boundingClientRect is DOMRectReadOnly type and we might modify the rect below. So we have to copy each attribute.

Comment 70

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Morris Tseng [:mtseng] from comment #68)
> (In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails,
> needinfo? please) from comment #65)
> > I think what you can do here is to call
> > <targetIframe>.dispatchEvent('mozContentEvent', ...) and retrieve the event
> > target in shell.js by loooking at event.target. Or dispatch the event on the
> > system app window, for the system app case.
> > 
> > Basically dispatching the event on the right target is a way to workaround
> > the limitation of cloneInto. We do that in devtools.js and the
> > PermissionPromptHelper.
> > 
> I have trouble with how to get <targetIframe>. For example, I use browser
> app to navigate a website, so the dom hierarchy look like this:
> 
> <shell.html>
>   <systemapp> (mozbrowser, mozapp)
>     <browserapp> (mozbrowser, mozapp)
>       <tab> (mozbrowser, remote)
> 
> When selection change event occurred, BrowserElementParent of <tab> dispatch
> 'mozbrowserselectionchange' event to its _frameElement. And shell.js
> receives this event then shell.js dispatch 'mozChromeEvent' to systemapp and
> the bubble shows. When I click on bubbles, systemapp should send
> 'mozContentEvent' to <targetIFrame>. In this case, correct <targetIFrame> is
> <tab>. But how can I get <tab> frame in the systemapp? I'm stuck with this
> problem, can you give some suggestion? Thanks.

Oh right :(

Then let's go with the helper thing for now, and let's figure out a better solution in a followup.

Comment 71

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

If you fix my comments about cancellable and show, I think we are good to go then.

Comment 72

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Implement mozbrowserselectionchange v4 (carry r+: ehsan)

Thanks for quick reply. Here is latest patch.

Comment 73

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

try push: https://tbpl.mozilla.org/?tree=Try&rev=7331a0e9c127

Comment 74

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8442751 [details] [diff] [review]
Implement mozbrowserselectionchange v4 (carry r+: ehsan)

Review of attachment 8442751 [details] [diff] [review]:
-----------------------------------------------------------------

r+ wit nits.

::: b2g/chrome/content/shell.js
@@ +514,5 @@
>          this.notifyContentStart();
>         break;
>  
> +      case 'mozbrowserselectionchange':
> +        // We might across chrome-content boundary here, so get the actual offsets.

// The mozbrowserselectionchange event, may have cross chrome-content boundary.
// So get the actual offsets for... [explain here why]

@@ +524,5 @@
> +        let rect = elt.getBoundingClientRect();
> +        offsetX += rect.left;
> +        offsetY += rect.top;
> +        evt.detail.frameOffsetX = offsetX;
> +        evt.detail.frameOffsetY = offsetY;

nit: s/frameOffset/offset ?

Comment 75

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8442751 [details] [diff] [review]
Implement mozbrowserselectionchange v4 (carry r+: ehsan)

Review of attachment 8442751 [details] [diff] [review]:
-----------------------------------------------------------------

Oups. Really r+'ing now.

Comment 76

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: mochitest for copy paste v3 (carry r+: ehsan)

The text in password field will be replaced by unicode "\u2022" in ubuntu but maybe be replaced by other character in other platform such as Windows. So we will fail in some platforms. So right now I just check two string have the same length only.

Comment 77

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Implement mozbrowserselectionchange v5 (carry r+: vingtetun)

Ehsan, I found nsRange::CollectClientRects will call FlushPendingNotifications [1]. Then our modification of nsGlobalWindow::UpdateCommands will the CollectClientRects which will cause some unexpected result in test. So I modify our SelectionChangeEvent slightly. Right now I embed Selection to the event instead of boundingClientRect and selectedText. Can you review my changes again? Thanks.

try push: https://tbpl.mozilla.org/?tree=Try&rev=4da3df78c89b


[1]: http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsRange.cpp?from=nsRange.cpp#2809

Comment 78

[(no longer active)]

Comment on attachment 8443360 [details] [diff] [review]
Implement mozbrowserselectionchange v5 (carry r+: vingtetun)

Review of attachment 8443360 [details] [diff] [review]:
-----------------------------------------------------------------

This will expose the internal selection objects that Gecko uses for <input> and <textarea> to content :(  While that is _probably_ OK, I think it's risky, so I'd rather not do this.  What goes wrong when we flush?  Perhaps we can solve that some other way, or add a non-flushing version of nsRange::CollectClientRects?  It's a little bit surprising to me that our layout information is out of date in nsGlobalWindow::UpdateCommands...

Comment 79

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

http://dxr.mozilla.org/mozilla-central/source/content/html/content/test/forms/test_input_textarea_set_value_no_scroll.html?from=test_input_textarea_set_value_no_scroll.html&case=true#81

In this test, the snapshot should contain textarea with "a\nb\nc\nd" but I got textarea with empty content when we call CollectClientRects in nsGlobalWindow::UpdateCommands. Most of try failure in comment 73 is related to this. I think our layout information should be update to date. So I'll try to add a non-flushing version and try again. Thanks

Comment 80

[(no longer active)]

(In reply to comment #79)
> http://dxr.mozilla.org/mozilla-central/source/content/html/content/test/forms/test_input_textarea_set_value_no_scroll.html?from=test_input_textarea_set_value_no_scroll.html&case=true#81
> 
> In this test, the snapshot should contain textarea with "a\nb\nc\nd" but I got
> textarea with empty content when we call CollectClientRects in
> nsGlobalWindow::UpdateCommands. Most of try failure in comment 73 is related to
> this. I think our layout information should be update to date. So I'll try to
> add a non-flushing version and try again. Thanks

Oh, then you need to debug what's happening there, I think.  Why do you think the right solution is to avoid flushing?

Comment 81

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Ehsan, I did some investigation. In that testcase, we set value of textarea 3 times. In second time, SetNeedLayoutFlush will be called. Then nsGlobalWindow::UpdateCommands calls GetBoundingClientRect() which flushes layout because mNeedLayoutFlush is true. But in the third time of setting value the SetNeedLayoutFlush won't be called so the screenshot is wrong content. I add SetNeedLayoutFlush() after calling GetBoundingClientRect() at nsGlobalWindow::UpdateCommands manually and the result is correct. So I think there is something wrong here. Do you know why changing the value of textarea doesn't trigger SetNeedLayoutFlush?

Comment 82

[(no longer active)]

Who's calling SetNeedLayoutFlush in the second case?  Perhaps there is a call missing to that function where we should be calling it.  Note that I don't actually know when we're supposed to call that function, so roc might be a better person to weigh in on that.

Comment 83

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Setting the textarea value should be calling through editor stuff into InsertTextTxn::DoTransaction() and then further down into sGenericDOMDataNode::SetTextInternal then down into nsCSSFrameConstructor::CharacterDataChanged and then into nsTextFrame::CharacterDataChanged which should call PresShell::FrameNeedsReflow which should call SetNeedLayoutFlush.

Comment 84

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Ehsan, I think I don't have trivial solution for this failure. The main problem is we call FlushPendingNotification at nsGlobalWindow::UpdateCommands but this flush didn't do "ACTUAL" flush and then some flags were wrong so that in the next draw gecko will think the layout is ready but it not ready. Therefore, we got error screenshot. So I think we shouldn't call FlushPendingNotification at UpdateCommands. Do you agree with me? If yes, then we have some options here.

1. Add a CollectClientRect which not calling FlushPendingNotification.
2. Can we directly send Range object instead of Selection object? I think Range object is safer than Selection object. At least Range object is part of standard.

What's your opinion? Thanks!

Comment 85

[(no longer active)]

(In reply to Morris Tseng [:mtseng] from comment #84)
> Ehsan, I think I don't have trivial solution for this failure. The main
> problem is we call FlushPendingNotification at
> nsGlobalWindow::UpdateCommands but this flush didn't do "ACTUAL" flush and
> then some flags were wrong so that in the next draw gecko will think the
> layout is ready but it not ready.

I'm not sure if we have flushes that don't do an "actual" flush...  I think we're hitting a bug here.  Did you follow what happens in the test and compare it against comment 83?

> Therefore, we got error screenshot. So I
> think we shouldn't call FlushPendingNotification at UpdateCommands. Do you
> agree with me? If yes, then we have some options here.

I agree that calling FlishPendingNotifications in UpdateCommands is probably wallpapering over a real bug elsewhere, so we shouldn't do that.

> 1. Add a CollectClientRect which not calling FlushPendingNotification.
> 2. Can we directly send Range object instead of Selection object? I think
> Range object is safer than Selection object. At least Range object is part
> of standard.

I don't think we should do either of these without understanding what bug we're working around at least.  Please note that I'd be fine working around a bug if we determine that it's not very important, but without any knowledge on what's happening here, we should assume that this would be something that the user might run into, so I'd rather focus on figuring out the bug first before focusing on how to work around it.  :-)

Comment 86

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: layout_not_flush

Yes, I trace layout update process in the test. I found we reset mNeedLayoutFlush [1] in the nsDocument::FlushLayoutNotification. But flush may not success due to some reason. For example, [2] shows we don't flush if we are not safe to flush. So that next flush command doesn't flush layout because mNeedLayoutFlush is false. So I set mNeedLayoutFlush to true if flush failed in this patch. roc, do you think this patch is on the right track?

[1]: http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp?from=nsDocument.cpp#7824
[2]: http://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#4137

Comment 87

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8445651 [details] [diff] [review]
layout_not_flush

Review of attachment 8445651 [details] [diff] [review]:
-----------------------------------------------------------------

This seems reasonable. However, it's probably better for PresShell::FlushPendingNotifications to return a bool to indicate whether it actually flushed or not. If it did not, then nsDocument::FlushPendingNotifications should not change mNeedStyleFlush and mNeedLayoutFlush.

Comment 88

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Although some dependent bugs are resolved. But there are few test fail. I'll update later.

Comment 89

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: browser_tabopen_reflows

Test location: browser/base/content/test/general/browser_tabopen_reflows.js

This test failed because select also cause reflow after applied our patch. The solution is easy, just added select to the expected reflow lists.

Comment 90

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: test_bug611182

Test location: editor/libeditor/html/tests/test_bug611182.html

This failure happened when document type is xml. When we create <head> node and append it to DOM tree, it will trigger nsGlobalWindow::UpdateCommands and layout flushed. Then nsXMLContentSink::FlushPendingNotification called and then nsXMLContentSink::MaybeStartLayout called.

I found that if we call MaybeStartLayout here then layout becomes very weird. So I added additional condition to avoid call MaybeStartLayout when <head> is attached.

Comment 91

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: test_bug586662

Test location: editor/libeditor/base/tests/test_bug586662.html

In nsEditor::EndPlaceHolderTransaction() will call EndUpdateViewBatch which will trigger selection change. So again this selection change will trigger layout flush by our patch. But this flush will get old value because HTMLTextAreaElement::mValueChanged is not set to true in this moment. HTMLTextAreaElement::mValueChanged will be set to true later when we call     NotifyEditorObservers().

So I think this is order problem. I move NotifyEditorObservers to the before EndUpdateViewBatch but I don't know whether this will cause another side effect.

Comment 92

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=c57864a95c3e

new try run after applying above 3 patches. Ehsan, do you have any suggestion about those try failures? Or we simply shouldn't flush layout at nsGlobalWindow::UpdateCommands :-)  Thanks.

Comment 93

[(no longer active)]

(In reply to Morris Tseng [:mtseng] from comment #92)
> https://tbpl.mozilla.org/?tree=Try&rev=c57864a95c3e
> 
> new try run after applying above 3 patches. Ehsan, do you have any
> suggestion about those try failures? Or we simply shouldn't flush layout at
> nsGlobalWindow::UpdateCommands :-)  Thanks.

These failures are scary enough for me to r- a patch that does flush during UpdateCommands.  :-)  Please note that the editor code is not super well tested, so these kinds of failures usually indicate even larger breakages in the stuff that we don't test.

Please file individual bugs about the test failures without flushing in UpdateCommands so that we can discuss them separately.  This bug is getting kind of confusing to follow...

Comment 94

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Implement mozbrowserselectionchange without flushing layout (carry r+: vingtetun)

Add a flag that indicate whether CollectClientRect should flush layout or not. So that we could avoid layout flush in nsGlobalWindow::UpdateCommands

Comment 95

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Implement mozbrowserselectionchange by sending nsRange (carry r+: vingtetun)

Send nsRange structure to BrowserElementChildPreload to avoid layout flush in nsGlobalWindow::UpdateCommands.

Comment 96

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Ehsan, I provided 2 version of patches for removing layout flush in UpdateCommands. First one is providing additional flag that indicate CollectClientRects should flush layout or not. Then we can get rect without flush layout by passing false for this flag. Second one is sending nsRange in SelectionChangeEvent. I have no idea which solution is better for now. Can you review both patches and give some opinion? Thanks.

Comment 97

[(no longer active)]

Comment on attachment 8449995 [details] [diff] [review]
Implement mozbrowserselectionchange without flushing layout (carry r+: vingtetun)

Review of attachment 8449995 [details] [diff] [review]:
-----------------------------------------------------------------

I've already reviewed this.  If you have made changes to the stuff I've reviewed before, please attach an interdiff so that I don't have to review the entire patch once again.  Thanks!

Comment 98

[(no longer active)]

(In reply to Morris Tseng [:mtseng] from comment #96)
> Ehsan, I provided 2 version of patches for removing layout flush in
> UpdateCommands. First one is providing additional flag that indicate
> CollectClientRects should flush layout or not. Then we can get rect without
> flush layout by passing false for this flag. Second one is sending nsRange
> in SelectionChangeEvent. I have no idea which solution is better for now.
> Can you review both patches and give some opinion? Thanks.

Err, I just saw this, sorry!  I prefer the solution of not flushing in CollectClientRects, but please attach an interdiff for review.  Thanks!

Comment 99

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 1: Implement mozbrowserselectionchange (carries r+: vingtetun, ehsan)

Rebase to latest m-c and update to address comment.

Comment 100

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 2: Add a flag that indicate layout flush or not in CollectClientRects

This patch prevent layout flush when get client rects.

Comment 101

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 3: Add a preference to toggle this api.

I add a additional preference so that we can disable this feature by pref.

Comment 102

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 4: Mochitest for this api (carry r+: ehsan)

Update commit message and enable the pref before starting test due to "part 3" set the pref default off.

Comment 103

[(no longer active)]

Comment on attachment 8450741 [details] [diff] [review]
Part 2: Add a flag that indicate layout flush or not in CollectClientRects

Review of attachment 8450741 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsRange.cpp
@@ +2773,5 @@
>  
>  static nsresult GetPartialTextRect(nsLayoutUtils::RectCallback* aCallback,
>                                     nsIContent* aContent, int32_t aStartOffset,
> +                                   int32_t aEndOffset,
> +                                   bool aClampToEdge, bool aFlushLayout)

Nit: please leave aClampToEdge on the previous line.

::: layout/base/SelectionCarets.cpp
@@ +381,5 @@
>  
>    nsLayoutUtils::FirstAndLastRectCollector collector;
>    nsRange::CollectClientRects(&collector, range,
>                                range->GetStartParent(), range->StartOffset(),
> +                              range->GetEndParent(), range->EndOffset(), true, true);

You don't need this change.

Comment 104

[(no longer active)]

Comment on attachment 8450742 [details] [diff] [review]
Part 3: Add a preference to toggle this api.

Review of attachment 8450742 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/app/b2g.js
@@ +988,5 @@
>  pref("touchcaret.enabled", false);
>  
>  // Disable selection caret by default
>  pref("selectioncaret.enabled", false);
> +pref("selectioncaret.fireselectionchange.enabled", false);

Why not just use selectioncaret.enabled?

::: dom/base/nsGlobalWindow.cpp
@@ +9272,5 @@
>      }
>    }
>  
> +  if (mDoc && anAction.EqualsLiteral("selectionchange") &&
> +      Preferences::GetBool("selectioncaret.fireselectionchange.enabled", false)) {

Please add a cache for this.

Comment 105

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #103)
> Comment on attachment 8450741 [details] [diff] [review]
> Part 2: Add a flag that indicate layout flush or not in CollectClientRects
> 
> Review of attachment 8450741 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/nsRange.cpp
> @@ +2773,5 @@
> >  
> >  static nsresult GetPartialTextRect(nsLayoutUtils::RectCallback* aCallback,
> >                                     nsIContent* aContent, int32_t aStartOffset,
> > +                                   int32_t aEndOffset,
> > +                                   bool aClampToEdge, bool aFlushLayout)
> 
> Nit: please leave aClampToEdge on the previous line.
Done.
> 
> ::: layout/base/SelectionCarets.cpp
> @@ +381,5 @@
> >  
> >    nsLayoutUtils::FirstAndLastRectCollector collector;
> >    nsRange::CollectClientRects(&collector, range,
> >                                range->GetStartParent(), range->StartOffset(),
> > +                              range->GetEndParent(), range->EndOffset(), true, true);
> 
> You don't need this change.
Done.

Comment 106

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #104)
> Comment on attachment 8450742 [details] [diff] [review]
> Part 3: Add a preference to toggle this api.
> 
> Review of attachment 8450742 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/app/b2g.js
> @@ +988,5 @@
> >  pref("touchcaret.enabled", false);
> >  
> >  // Disable selection caret by default
> >  pref("selectioncaret.enabled", false);
> > +pref("selectioncaret.fireselectionchange.enabled", false);
> 
> Why not just use selectioncaret.enabled?
hmm, I'll use this pref in next patch.
> 
> ::: dom/base/nsGlobalWindow.cpp
> @@ +9272,5 @@
> >      }
> >    }
> >  
> > +  if (mDoc && anAction.EqualsLiteral("selectionchange") &&
> > +      Preferences::GetBool("selectioncaret.fireselectionchange.enabled", false)) {
> 
> Please add a cache for this.
Done.

Comment 107

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 2: Add a flag that indicate layout flush or not in CollectClientRects v2 (carry r+: ehsan)

Comment 108

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 3: Add a preference to toggle this api v2

Update to address comment.

Comment 109

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=3c73d190df58

Comment 110

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Morris Tseng [:mtseng] from comment #105)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #103)
> > Comment on attachment 8450741 [details] [diff] [review]
> > Part 2: Add a flag that indicate layout flush or not in CollectClientRects
> > 
> > Review of attachment 8450741 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: layout/base/SelectionCarets.cpp
> > @@ +381,5 @@
> > >  
> > >    nsLayoutUtils::FirstAndLastRectCollector collector;
> > >    nsRange::CollectClientRects(&collector, range,
> > >                                range->GetStartParent(), range->StartOffset(),
> > > +                              range->GetEndParent(), range->EndOffset(), true, true);
> > 
> > You don't need this change.
> Done.
Oops, this cannot be done because of interface changed.

Comment 111

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 2: Add a flag that indicate layout flush or not in CollectClientRects v3 (carry r+: ehsan)

Fix build break.

Comment 112

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 3: Add a preference to toggle this api v3

Fix build break.

Comment 113

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

new try run: https://tbpl.mozilla.org/?tree=Try&rev=d2b954395912

Comment 114

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 4: Mochitest for this api v2 (carry r+: ehsan)

Update patch to reflect pref change

new try run: https://tbpl.mozilla.org/?tree=Try&rev=3674e1a6c0b4

Comment 115

[(no longer active)]

(In reply to Morris Tseng [:mtseng] from comment #110)
> (In reply to Morris Tseng [:mtseng] from comment #105)
> > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> > #103)
> > > Comment on attachment 8450741 [details] [diff] [review]
> > > Part 2: Add a flag that indicate layout flush or not in CollectClientRects
> > > 
> > > Review of attachment 8450741 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: layout/base/SelectionCarets.cpp
> > > @@ +381,5 @@
> > > >  
> > > >    nsLayoutUtils::FirstAndLastRectCollector collector;
> > > >    nsRange::CollectClientRects(&collector, range,
> > > >                                range->GetStartParent(), range->StartOffset(),
> > > > +                              range->GetEndParent(), range->EndOffset(), true, true);
> > > 
> > > You don't need this change.
> > Done.
> Oops, this cannot be done because of interface changed.

Ah, sorry, I thought that was an optional argument.  You're right!

Comment 116

[(no longer active)]

Comment on attachment 8451426 [details] [diff] [review]
Part 3: Add a preference to toggle this api v3

Review of attachment 8451426 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.cpp
@@ +1179,5 @@
>                                   "dom.idle-observers-api.fuzz_time.disabled",
>                                   false);
> +    Preferences::AddBoolVarCache(&gSelectionCaretPrefEnabled,
> +                                 "selectioncaret.enabled",
> +                                 false);

So now we have two places where we do this: <http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#727>  Please file a follow-up to refactor this into an nsContentUtils method, but this patch can land as is.

@@ +9275,5 @@
>                                                              anAction));
>      }
>    }
>  
> +  if (mDoc && anAction.EqualsLiteral("selectionchange") && gSelectionCaretPrefEnabled) {

Nit: check for the boolean flag first.

Comment 117

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 2: Add a flag that indicate layout flush or not in CollectClientRects v4 (carry r+: ehsan)

Update reviewer infomation to commit message.

Comment 118

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 3: Add a preference to toggle this api v4 (carry r+: ehsan)

Update to address comment and append reviewer information.

Comment 119

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

I think try run is good, ready for check-in.

Comment 120

[Ryan VanderMeulen [:RyanVM]]

Part 1 needs DOM peer review for the WebIDL changes.

Comment 121

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8450740 [details] [diff] [review]
Part 1: Implement mozbrowserselectionchange (carries r+: vingtetun, ehsan)

Hi bz,
This patch add a webidl file. Can you review it for me? Thanks.

Comment 122

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8450740 [details] [diff] [review]
Part 1: Implement mozbrowserselectionchange (carries r+: vingtetun, ehsan)

>+++ b/b2g/chrome/content/shell.js
>+        // The mozbrowserselectionchange event, may have cross chrome-content boundary.

"may have crossed the"

>+        // Ex: browser app's each tab.

I'm not sure what this is trying to say.  Please rephrase to make this clearer.

Do you really want to modify event.detail in-place?

>+      XPCNativeWrapper.unwrap(this._event.target).doCommand(cmd);

Why do you need XPCNativeWrapper.unwrap() here?  That seems pretty odd to me.  At the very least, it should be clearly documented to make it clear why exactly we need this and that the security implications have been considered (and in particular why this is not a security issue).

>+++ b/content/html/content/src/nsTextEditorState.cpp
>+                                    short reason = 0);

int16_t, please.  Same thing in the implementation.

>+++ b/dom/base/nsGlobalWindow.cpp
>+  // selectionchange action only use for mozbrowser, not for XUL. So we bypass

"is only used", perhaps?

>+  // XUL command dispatch if it is selectionchange action.

Which "it"?  Maybe 'if anAction is "selectionchange"'?

>+++ b/dom/webidl/SelectionChangeEvent.webidl

>+  DOMString? selectedText = "";

Why is this nullable?  Nothing seems to ever set it to null.

>+  short? reason = 0;

Likewise.

>+  readonly attribute DOMString? selectedText;
>+  readonly attribute short? reason;

And these two, therefore.

>+++ b/layout/base/nsDocumentViewer.cpp
>+  nsPIDOMWindow *domWindow = theDoc->GetWindow();

This needs to be a strong reference, since you use it again after event dispatch.

sr=me with those fixed

Comment 123

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8450740 [details] [diff] [review]
Part 1: Implement mozbrowserselectionchange (carries r+: vingtetun, ehsan)

Olli, does the 

>+      event->GetInternalNSEvent()->mFlags.mOnlyChromeDispatch = true;

bit and cod surrounding it make sense?

Comment 124

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8450740 [details] [diff] [review]
Part 1: Implement mozbrowserselectionchange (carries r+: vingtetun, ehsan)

event->GetInternalNSEvent()->mFlags.mOnlyChromeDispatch = true; as such is fine, 
but why are we adding chromeonly selectionchange?
That is an event type which webkit and trident support for web content.
Would be safer to use some other event name.

Comment 125

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Boris Zbarsky [:bz] from comment #122)
> Comment on attachment 8450740 [details] [diff] [review]
> Part 1: Implement mozbrowserselectionchange (carries r+: vingtetun, ehsan)
> 
> >+++ b/b2g/chrome/content/shell.js
> >+        // The mozbrowserselectionchange event, may have cross chrome-content boundary.
> 
> "may have crossed the"
Done.
> 
> >+        // Ex: browser app's each tab.
> 
> I'm not sure what this is trying to say.  Please rephrase to make this
> clearer.
Ok, I added more detail information.
> 
> Do you really want to modify event.detail in-place?
Yes, in this case we need those extra informations.
> 
> >+      XPCNativeWrapper.unwrap(this._event.target).doCommand(cmd);
> 
> Why do you need XPCNativeWrapper.unwrap() here?  That seems pretty odd to
> me.  At the very least, it should be clearly documented to make it clear why
> exactly we need this and that the security implications have been considered
> (and in particular why this is not a security issue).
hmm, I use another way to avoid unwrap here.
> 
> >+++ b/content/html/content/src/nsTextEditorState.cpp
> >+                                    short reason = 0);
> 
> int16_t, please.  Same thing in the implementation.
Done.
> 
> >+++ b/dom/base/nsGlobalWindow.cpp
> >+  // selectionchange action only use for mozbrowser, not for XUL. So we bypass
> 
> "is only used", perhaps?
Done.
> 
> >+  // XUL command dispatch if it is selectionchange action.
> 
> Which "it"?  Maybe 'if anAction is "selectionchange"'?
Done
> 
> >+++ b/dom/webidl/SelectionChangeEvent.webidl
> 
> >+  DOMString? selectedText = "";
> 
> Why is this nullable?  Nothing seems to ever set it to null.
Done
> 
> >+  short? reason = 0;
> 
> Likewise.
Done
> 
> >+  readonly attribute DOMString? selectedText;
> >+  readonly attribute short? reason;
> 
> And these two, therefore.
Done
> 
> >+++ b/layout/base/nsDocumentViewer.cpp
> >+  nsPIDOMWindow *domWindow = theDoc->GetWindow();
> 
> This needs to be a strong reference, since you use it again after event
> dispatch.
Done
> 
> sr=me with those fixed
Thanks!

Comment 126

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 1: Implement mozbrowserselectionchange v2 (carries r+: vingtetun, ehsan sr+:bz)

Update to address comment.

Hi Olli,
I changed the event name from "selectionchange" to "mozselectionchange". Do you think this is proper name?

Comment 127

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 2: Add a flag that indicate layout flush or not in CollectClientRects v5 (carry r+: ehsan)

Rebased.

Comment 128

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 3: Add a preference to toggle this api v5 (carry r+: ehsan)

Rebased

Comment 129

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 4: Mochitest for this api v3 (carry r+: ehsan)

Rebased.

Comment 130

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8452921 [details] [diff] [review]
Part 1: Implement mozbrowserselectionchange v2 (carries r+: vingtetun, ehsan sr+:bz)

That is better.

Comment 131

[Boris Zbarsky [:bzbarsky]]

> Yes, in this case we need those extra informations.

In that case, please add a comment explaining what other code that examines this event object depends on it being modified in this way.  And if needed, a comment in that code pointing out the dependency on this bit.

Comment 132

[Boris Zbarsky [:bzbarsky]]

>+        // Ex: browser app's each tab is remote frame but isn't mozapp. No one handle this

I think my issue here is with "browser app's each tab".  Do you mean "For example, tabs in the browser app are remote iframes but aren't mozapp iframes.  They don't handle the mozbrowserselectionchange event, so it ends up propagating to shell.js.  But the offset ..."

Why is the __exposedProps__ bit needed?  We really shouldn't be adding __exposedProps__ usage...

Comment 133

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Boris Zbarsky [:bz] from comment #132)
> >+        // Ex: browser app's each tab is remote frame but isn't mozapp. No one handle this
> 
> I think my issue here is with "browser app's each tab".  Do you mean "For
> example, tabs in the browser app are remote iframes but aren't mozapp
> iframes.  They don't handle the mozbrowserselectionchange event, so it ends
> up propagating to shell.js.  But the offset ..."
Hmm, this is exactly what I mean. I'll modify it for easily read.
> 
> Why is the __exposedProps__ bit needed?  We really shouldn't be adding
> __exposedProps__ usage...
Because defineNoReturnMethod function only define function at unwrap object. But shell.js event handler get wrapped object. So I need either use __exposedProps__ to expose function to wrapped object or use XPCNativeWrapper.unwrap() which has some security concern. Does it have better solution that doesn't use __exposeProps__ and XPCNativeWrapper.unwrap() here?

Comment 134

[Boris Zbarsky [:bzbarsky]]

> But shell.js event handler get wrapped object. 

What principals is shell.js running with?  What principals is the code that does defineNoReturnMethod running with?

Comment 135

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Basically, I want embedder able to inform gecko which command (cut/copy/paste/selectall) we want to do. So in the BrowserElementParent.jsm, I define a method "doCommand" which simply call sendAsyncMsg to send "do-command" message to child. This method is defined by "defineNoReturnMethod" which defined function in the unwrap object of frameElemnt.

In the app which is managed by AppWindowManager in gaia, we can simply get unwrap object of frameElement mentioned above and call doCommand by this code "this.app.iframe.doCommand()".

But in the app with remote iframe which is not mozapp iframe means this iframe is not managed by AppWindowManager. So all mozbrowserselectionchange event will propagate to shell.js. The only way I can tell which frameElement is what I want is through event.target. But event.target is wrapped object of frameElement, I cannot call event.target.doCommand() directly. I need call unwrap before calling doCommand function. That why I use __exposeProps__ to define function in wrapped object and avoid unwrap operation.

Comment 136

[Boris Zbarsky [:bzbarsky]]

> This method is defined by "defineNoReturnMethod" which defined function in the unwrap
> object of frameElemnt.

OK, so just to make sure I understand:

1)  BrowserElementParent.jsm is running with the system principal.
2)  It wants to expose an API to an untrusted page (whatever page this._frameElement is
    in).
3)  It does this right now by directly sticking a system-principal function on the
    frameElement (unwrapping Xrays as needed to do that).  This seems a bit fishy to me;
    Bobby, is this really how we're supposed to do these things nowadays?
4)  Later untrusted code calls this function to do some privileged work on its behalf.
5)  We also want to call this same function from other system principal code (shell.js).

Is this understanding correct? If so, some questions:

I. How does shell.js make sure that it's calling the function it expects and not
   whatever function the untrusted page decided to stick on the object?
II. Is there a reason you can't define the function both on the Xray (for shell.js to
    use) and on the underlying content object for the untrusted page to use?

Comment 137

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #136)
> > This method is defined by "defineNoReturnMethod" which defined function in the unwrap
> > object of frameElemnt.
> 
> OK, so just to make sure I understand:
> 
> 1)  BrowserElementParent.jsm is running with the system principal.
> 2)  It wants to expose an API to an untrusted page (whatever page
> this._frameElement is
>     in).
> 3)  It does this right now by directly sticking a system-principal function
> on the
>     frameElement (unwrapping Xrays as needed to do that).  This seems a bit
> fishy to me;
>     Bobby, is this really how we're supposed to do these things nowadays?

No. It was done this was as a quick hack, and I was promised that it would be improved at some point, but that didn't happen (probably because the relevant people aren't working here anymore).

Adding new usage of __exposedProps__ is banned at this point. Gijs specifically ripped it out of the BrowserElement API in bug 1019643, so we definitely shouldn't be adding more here. A combination of cloneInto and exportFunction should suffice for anything that needs to happen here.

Long-term, we should really figure out how to fix up the BrowserElement API to do something saner.

Comment 138

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Bobby Holley (:bholley) from comment #137)
> (In reply to Boris Zbarsky [:bz] from comment #136)
> > > This method is defined by "defineNoReturnMethod" which defined function in the unwrap
> > > object of frameElemnt.
> > 
> > OK, so just to make sure I understand:
> > 
> > 1)  BrowserElementParent.jsm is running with the system principal.
> > 2)  It wants to expose an API to an untrusted page (whatever page
> > this._frameElement is
> >     in).
> > 3)  It does this right now by directly sticking a system-principal function
> > on the
> >     frameElement (unwrapping Xrays as needed to do that).  This seems a bit
> > fishy to me;
> >     Bobby, is this really how we're supposed to do these things nowadays?
> 
> No. It was done this was as a quick hack, and I was promised that it would
> be improved at some point, but that didn't happen (probably because the
> relevant people aren't working here anymore).
> 
> Adding new usage of __exposedProps__ is banned at this point. Gijs
> specifically ripped it out of the BrowserElement API in bug 1019643, so we
> definitely shouldn't be adding more here. A combination of cloneInto and
> exportFunction should suffice for anything that needs to happen here.
I'm trying use exportFunction, but I get some trouble about using it. I'll explain detail in next commnet.
> 
> Long-term, we should really figure out how to fix up the BrowserElement API
> to do something saner.

Comment 139

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 1: Implement mozbrowserselectionchange v2 (carries r+: vingtetun, ehsan, bugs)

Hi bz, I made some changes about this new patch.

1. I set "useCapture" to true when I listen to "mozbrowserselectionchange" event in shell.js and then shell.js call stopPropagation on this event once shell.js receive this event. In other words, "mozbrowserselectionchange" only send to shell.js.

2. Instead use "defineNoReturnMethod" to define function on frameElement, I define function on the event's detail by using "Cu.exportFunction" as Bobby suggested.

Above modifications which mean the privileged code should not expose to untrusted page. Do you think this is good? Thanks.

Comment 140

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8454313 [details] [diff] [review]
Part 1: Implement mozbrowserselectionchange v2 (carries r+: vingtetun, ehsan, bugs)

This still has the original problem I commented on: DoCommandHelper is invoking an untrusted doCommand function.  Why is that safe/desirable?

Comment 141

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 1: Implement mozbrowserselectionchange v3 (carries r+: vingtetun, ehsan, bugs)

Hi bz, 
Maybe we can check the principal of our document and target's document before calling the untrusted "doCommand" function. Do you think this is sufficient? I have updated the patch for this change.

Comment 142

[Boris Zbarsky [:bzbarsky]]

I don't understand how the principal check you added is helping us.

I mean, I assume this._event.target.ownerDocument == getContentWindow().document, no?

The issue is that it's not clear to me why we're trusting that document not to have overridden the doCommand bit.

I would also somewhat appreciate an answer to my question at the end of comment 136.

Comment 143

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Boris Zbarsky [:bz] from comment #142)
> I don't understand how the principal check you added is helping us.
> 
> I mean, I assume this._event.target.ownerDocument ==
> getContentWindow().document, no?
ah yes, this code didn't help anything! I have another thought about this. Details below.

> 
> The issue is that it's not clear to me why we're trusting that document not
> to have overridden the doCommand bit.
Hmm, but I think since our apps are all OOP, they may not have any change to overridden the doCommand bit. Am I correct? Right now the doCommand is stick with evt.detail. The only way to fake doCommand is sending a fake mozbrowserselectionchange event from untrusted page. But OOP apps unable to sending this event to shell.js since the process is different.
> 
> I would also somewhat appreciate an answer to my question at the end of
> comment 136.
hmm I think Q1 is critical but I have no idea about how we make sure calling doCommand is safe. As my comment above, maybe doCommand is not able to override.
Q2 is not valid since I send all mozbrowserselectionchange to shell.js only.

Comment 144

[Boris Zbarsky [:bzbarsky]]

> they may not have any change to overridden the doCommand bit. Am I correct? 

I have no idea.

> The only way to fake doCommand is sending a fake mozbrowserselectionchange event from
> untrusted page.

Or intercept a real one and change its .detail, right?

> But OOP apps unable to sending this event to shell.js since the process is different.

Then why is there an untrusted scope at all that shell.js is seeing?  What is that untrusted global, exactly.

> Q2 is not valid since I send all mozbrowserselectionchange to shell.js only.

Comment 135 specifically talks about us having both trusted and untrusted callers that want to call doCommand.  My question is why we don't just provide them different doCommand functions instead of sending them both through the function in the untrusted scope.

Comment 145

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 1: Implement mozbrowserselectionchange v3 (carries r+: vingtetun, ehsan, bugs)

Hi bz, instead of exporting function which causes some security issues, I change my implementation to using event passing. So that I can get rid of call unsafe function in shell.js. Do you think this is ok? Thanks!

Comment 146

[Boris Zbarsky [:bzbarsky]]

Yes, that looks much better!

Comment 147

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 1: Implement mozbrowserselectionchange v4 (carries r+: vingtetun, ehsan, bugs. sr+:bz)

Rebased.

Comment 148

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 2: Add a flag that indicate layout flush or not in CollectClientRects v6 (carry r+: ehsan)

Rebased.

Comment 149

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 1: Implement mozbrowserselectionchange v5 (carries r+: vingtetun, ehsan, bugs. sr+:bz)

Update commit message.

Comment 150

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 4: Mochitest for this api v4 (carry r+: ehsan)

Rebased and some modification due to part 1 change.

Comment 151

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

try run: https://tbpl.mozilla.org/?tree=Try&rev=18dca2eabeac

Comment 152

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 4: Mochitest for this api v5 (carry r+: ehsan)

Fix try server failed.

new try run: https://tbpl.mozilla.org/?tree=Try&rev=0bc85af10211

Comment 153

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 1: Implement mozbrowserselectionchange v6 (carries r+: vingtetun, ehsan, bugs. sr+:bz)

Fix mochitest failure

Comment 154

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 4: Mochitest for this api v6 (carry r+: ehsan)

Fix mochitest failure. 
try run: https://tbpl.mozilla.org/?tree=Try&rev=07be5ff1ae14

Comment 155

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc734b3bbb42
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2d5fa0473cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6dcb9fb9062
https://hg.mozilla.org/integration/mozilla-inbound/rev/d308a4e99f78

Comment 156

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fc734b3bbb42
https://hg.mozilla.org/mozilla-central/rev/c2d5fa0473cb
https://hg.mozilla.org/mozilla-central/rev/f6dcb9fb9062
https://hg.mozilla.org/mozilla-central/rev/d308a4e99f78

Comment 157

[u459114]

awesome!

Comment 158

[Neil Deakin]

Comment on attachment 8463260 [details] [diff] [review]
Part 1: Implement mozbrowserselectionchange v6 (carries r+: vingtetun, ehsan, bugs. sr+:bz)

># HG changeset patch
># Parent a91ec42d6a9cd718673372a2c70bfca7903c3ca6
># User Morris Tseng <[e-mail]>
>Bug 987040 - Part 1: Implement mozbrowserselectionchange. r=vingtetun,ehsan,bugs. sr=bz
>

This patch doesn't make any sense. You make an entirely unnecessary API change to UpdateCommands, then use this to add code to it to do something that doesn't have anything to do with updating commands.