Add option to exclude ruby annotation from plain text when copied

NEW
Assigned to

Status

()

enhancement
P5
normal
5 years ago
2 months ago

People

(Reporter: alice0775, Assigned: xidorn, NeedInfo)

Tracking

(Blocks 1 bug, {leave-open})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

When I select text rupy layout. It contains whole tag related rupy.

So search with selected text to work properly, I think that it is need to select Ruby base only .

Please provide an option to select only Ruby base.

Steps To Reproduce:
1. Open  ex. http://www.w3.org/International/tests/repository/run?manifest=html5/the-ruby-element&test=ruby-styling-001

  <ruby><rb>A</rb><rt>aaa</rt><rb>B</rb><rt>bb</rt><rb>C</rb><rt>cc</rt><rb>D</rb><rt>d</rt></ruby>...

2. Attempt to select ABCD
3. Copy to clipboard the above selected text / Search with the above selected text

Actual Results:
AaaaBbbCccDd is selected / copied.
It is searched with "AaaaBbbCccDd" by default search engine

Expected Results:
ABCD should be selected / copied
It should be searched with "ABCD" by default search engine
It might be hard to find an unused modifier key combination to alter the selection
behavior for this (and it wouldn't be very discoverable either).

A couple of alternatives:
A. select as usual (including ruby text), then offer a "Unselect ruby text" in
   the context menu (if the selection contains ruby)
B. select as usual (including ruby text), then offer "Copy, excluding ruby text"
   in the context menu (if the selection contains ruby)
Status: UNCONFIRMED → NEW
Component: Layout → Selection
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
I'd suggest it select only base by default, and provide an optional method to also include the annotation. Actually, I guess we can always ignore ruby annotation, and provide an addon to inlinize all ruby annotation for selecting for best effect. Does it sound like something we should do?
(In reply to Xidorn Quan [:xidorn] (UTC+13) from comment #2)
> I'd suggest it select only base by default,

If the base text is what the user wants to copy/paste in most cases then I guess
that's what we should provide as the default.  I'm not familiar enough with
the use of Ruby text myself so I don't know.

> and provide an optional method to also include the annotation.

OK, but I think it should be built-in and provided through the context menu,
not by an additional add-on (unless it's exceptionally rare to also want to
copy the annotation).

For example when copy/pasting into an HTML editor, e.g. Thunderbird, Gmail,
GoogleDocs etc, I suspect that you always want to copy all the markup,
including the annotations, right?
(In reply to Mats Palmgren (:mats) from comment #3)
> (In reply to Xidorn Quan [:xidorn] (UTC+13) from comment #2)
> > I'd suggest it select only base by default,
> 
> If the base text is what the user wants to copy/paste in most cases then I
> guess
> that's what we should provide as the default.  I'm not familiar enough with
> the use of Ruby text myself so I don't know.

I've sent an email to w3c international group to ask about this. [1] Let's see what other people think about.

> > and provide an optional method to also include the annotation.
> 
> OK, but I think it should be built-in and provided through the context menu,
> not by an additional add-on (unless it's exceptionally rare to also want to
> copy the annotation).

Well, what I was thinking is, the addon can not only make the ruby content be inlinized (make <ruby>/<rb>/<rt>/<rtc> use display: inline), but also add bracket around if there are neither <rp> nor ::before/::after on <rt>/<rtc>. I don't think it is something we should do in Gecko. But in Firefox, I guess it's probably OK. But it may still cause problem. That's why I think it might be better to live in an addon rather than the browser.

> For example when copy/pasting into an HTML editor, e.g. Thunderbird, Gmail,
> GoogleDocs etc, I suspect that you always want to copy all the markup,
> including the annotations, right?

Yes, so I guess the best way would be that the ruby annotation is included only in the HTML part, but not the plain text part when copy/pasting. Is it something doable in our code? I guess it should be, but haven't investigated deeply.

[1] https://lists.w3.org/Archives/Public/www-international/2015JanMar/0072.html
So, masayuki, could you answer that, how often do you think people want to select the annotation as well as the base text for ruby? And if they do so, what do you think they may expect for the content in the clipboard/pasteboard?
Flags: needinfo?(masayuki)
In addition to the reason I mentioned above, an addon can provide more options about serialization for people with special requirements, and can be updated more frequently than the browser itself.
(In reply to Xidorn Quan [:xidorn] (UTC+13) from comment #5)
> So, masayuki, could you answer that, how often do you think people want to
> select the annotation as well as the base text for ruby?

Not sure.

There are two scenarios.

One is copying as HTML fragment. In this case, ruby text should be copied because users may except keep the ruby structure in pasted editor.

The other is copying as plaintext. In this case, there are two possibilities of most users. One is want to select only ruby base. The other is, trying to copy *only* ruby text.

> And if they do so,
> what do you think they may expect for the content in the
> clipboard/pasteboard?

Ideally, i.e., not thinking about the complicated logic of implementation, the root element of selection should be limited when copying only in a ruby text box.  Otherwise, ruby text contents should be ignored. But I'm not sure if this can easy to be implemented.
Flags: needinfo?(masayuki)
> One is copying as HTML fragment. In this case, ruby text should be copied because users may except keep the ruby structure in pasted editor.

s/may except/may expect
Attachment #8571291 - Flags: review?(roc)
Attachment #8571293 - Flags: review?(bugs)
This patchset implements this:
1. still select everything, because we rely on this to make HTML markups copy and removal behave correctly;
2. ruby annotations are stripped out from the plain text copied by default, and <rp>s are now included in the copied HTML fragment;
3. a new context menu item "Copy with Ruby" is added when there is any ruby annotation in the document, which adds both <rp>s and ruby annotations to the copied plain text.

It should work fine for most cases.
Comment on attachment 8571293 [details] [diff] [review]
patch 3 - add context menu item

This definitely needs a Firefox peer review.
('Copy with Ruby' sounds odd to me. Do users understand what that means?)

NS_COPY_WITH_RUBY event doesn't make sense.
NS_COPY/CUT/PASTE map to "copy"/"cut"/"paste" DOM Events. 
NS_COPY_WITH_RUBY doesn't map to anything, nor is there copyWithRuby event in any spec.
Attachment #8571293 - Flags: review?(bugs) → review-
Attachment #8571290 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #13)
> Comment on attachment 8571293 [details] [diff] [review]
> patch 3 - add context menu item
> 
> This definitely needs a Firefox peer review.
> ('Copy with Ruby' sounds odd to me. Do users understand what that means?)

I'm not sure. I guess they do, but I'm open for a better name.

> NS_COPY_WITH_RUBY event doesn't make sense.
> NS_COPY/CUT/PASTE map to "copy"/"cut"/"paste" DOM Events. 
> NS_COPY_WITH_RUBY doesn't map to anything, nor is there copyWithRuby event
> in any spec.

Does it sound better to add a bool parameter with default value to FireClipboardEvent?
Attachment #8571293 - Flags: review?(mconley)
Yeah, a bool param should be fine.
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #12)
> This patchset implements this:
> 1. still select everything, because we rely on this to make HTML markups
> copy and removal behave correctly;

Sounds good.

> 2. ruby annotations are stripped out from the plain text copied by default,
> and <rp>s are now included in the copied HTML fragment;

I'd rather avoid adding new UI for this, if possible.

Based on Masayuki's comment #7, how about this: if the selection is entirely inside a single ruby container, preserve the ruby text when converting to plaintext.
Comment on attachment 8571291 [details] [diff] [review]
patch 2 - add flag in document encoder to control

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

See comment in bug.
Attachment #8571291 - Flags: review?(roc) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #12)
> > This patchset implements this:
> > 1. still select everything, because we rely on this to make HTML markups
> > copy and removal behave correctly;
> 
> Sounds good.
> 
> > 2. ruby annotations are stripped out from the plain text copied by default,
> > and <rp>s are now included in the copied HTML fragment;
> 
> I'd rather avoid adding new UI for this, if possible.
> 
> Based on Masayuki's comment #7, how about this: if the selection is entirely
> inside a single ruby container, preserve the ruby text when converting to
> plaintext.

Sounds good. But one thing I'm concerned is that the user might accidentally select extra text outside the ruby container, e.g. a space before. It could make them confused for this behavior.
> if the selection is entirely inside a single ruby container, preserve the ruby text when
> converting to plaintext.

I suspect that would be hard for users to discover and understand.  It's also less functional
since you can't copy a larger chunk of text with Ruby annotations included.  Xidorn's context-
menu proposal sounds good to me - it's reasonably easy to discover and offers full functionality.

We should probably let UX people decide this though. ;-)
(In reply to Mats Palmgren (:mats) from comment #19)
> > if the selection is entirely inside a single ruby container, preserve the ruby text when
> > converting to plaintext.
> 
> I suspect that would be hard for users to discover and understand.  It's
> also less functional
> since you can't copy a larger chunk of text with Ruby annotations included. 
> Xidorn's context-
> menu proposal sounds good to me - it's reasonably easy to discover and
> offers full functionality.
> 
> We should probably let UX people decide this though. ;-)

So how can I ask UX people about this?
Comment on attachment 8571293 [details] [diff] [review]
patch 3 - add context menu item

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

Hey Xidorn,

Just a little concerned that this is going to break with e10s. See my comment below.

Thanks,

-Mike

::: browser/base/content/nsContextMenu.js
@@ +1523,5 @@
>    },
>  
> +  // Returns true if there is any ruby annotation in the document.
> +  documentHasRuby: function() {
> +    return !!this.focusedWindow.document.querySelector("rt, rtc");

This is, at worse, not going to work with e10s, and at best, use unsafe CPOWs (which we're attempting to eliminate).

Instead of synchronously detecting whether the document has ruby here, you should probably put that detection somewhere in here: https://hg.mozilla.org/mozilla-central/file/0b3c520002ad/browser/base/content/content.js#l131

And then (in the e10s case), pass the value up through the message and set it in gContextMenuContentData here: https://hg.mozilla.org/mozilla-central/file/0b3c520002ad/browser/base/content/tabbrowser.xml#l3221

For the non-e10s case, it can just be set in gContextMenuContentData directly: https://hg.mozilla.org/mozilla-central/file/0b3c520002ad/browser/base/content/content.js#l175

Also, is it really necessary to use querySelector? Is there no better way for a document in Gecko to advertise that it possesses Ruby syntax without having to search the DOM?
Attachment #8571293 - Flags: review?(mconley) → review-
The best result would be, if there is any ruby in the selection, show that menu item. But I didn't figure out a good way to do that.

If we have to back to Gecko to have that information, I guess it would be better to think a bit about whether I can use an effective method to get the best result there directly. Let me try.
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #20)
> So how can I ask UX people about this?

ni? madhava. Is this "Copy with Ruby" context-menu item something you guys want to review? The current plan is to only display it when there's ruby markup in the text selection.
Flags: needinfo?(madhava)
(In reply to Mats Palmgren (:mats) from comment #19)
> > if the selection is entirely inside a single ruby container, preserve the ruby text when
> > converting to plaintext.
> 
> I suspect that would be hard for users to discover and understand.

I'm basing my analysis on Masayuki's comment #7:
> In this case, there are two possibilities of most users. One is want to select only ruby base.
> The other is, trying to copy *only* ruby text.

If you're trying to copy only ruby text, you probably will restrict your selection to a single ruby container.

> It's also less functional
> since you can't copy a larger chunk of text with Ruby annotations included.

According to Masayuki this is not common. Also remember that this is only about plaintext conversion. If you copy and paste HTML you will copy the text with annotations included.

> Xidorn's context-
> menu proposal sounds good to me - it's reasonably easy to discover and
> offers full functionality.

Maybe so, but we'll need additional UI for B2G and Android, and I'm not sure how that will work.
Keywords: leave-open
Summary: Add an option to select only Ruby base → Add option to exclude ruby annotation from plain text when copied
I discussed this with roc, he thought that just landing the first two patches is fine, but would be really unfortunate that if a user wants to copy the annotation only, there would be nothing in the clipboard for plain text.

But if we think copying annotation only is not what we need to consider at present, we can just land the two patches and leave everything else to be discussed later.
Attachment #8571293 - Attachment is obsolete: true
Attachment #8572426 - Flags: review?(bugs)
Comment on attachment 8572423 [details] [diff] [review]
patch 3 - Add param to HTMLCopy

># HG changeset patch
># User Xidorn Quan <quanxunzhen@gmail.com>
># Date 1425444873 -39600
>#      Wed Mar 04 15:54:33 2015 +1100
># Node ID c653134c9ffddc8ee765fd5df20a1c7aa4387609
># Parent  04ed6a5d351f678d5731ea98f1e4c4512a1a52bb
>Bug 1130891 part 3 - Add param to HTMLCopy to allow copy with ruby annotation.
>
>diff --git a/dom/base/nsCopySupport.cpp b/dom/base/nsCopySupport.cpp
>--- a/dom/base/nsCopySupport.cpp
>+++ b/dom/base/nsCopySupport.cpp
>@@ -143,17 +143,18 @@ SelectionCopyHelper(nsISelection *aSel, 
>     textPlainBuf.Assign(buf);
>   } else {
>     // Redo the encoding, but this time use pretty printing.
>     flags =
>       nsIDocumentEncoder::OutputSelectionOnly |
>       nsIDocumentEncoder::OutputAbsoluteLinks |
>       nsIDocumentEncoder::SkipInvisibleContent |
>       nsIDocumentEncoder::OutputDropInvisibleBreak |
>-      (aFlags & nsIDocumentEncoder::OutputNoScriptContent);
>+      (aFlags & (nsIDocumentEncoder::OutputNoScriptContent |
>+                 nsIDocumentEncoder::OutputRubyAnnotation));
> 
>     mimeType.AssignLiteral(kTextMime);
>     rv = docEncoder->Init(domDoc, mimeType, flags);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     rv = docEncoder->SetSelection(aSel);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>@@ -268,21 +269,23 @@ SelectionCopyHelper(nsISelection *aSel, 
>       }
>     }
>   }
>   return rv;
> }
> 
> nsresult
> nsCopySupport::HTMLCopy(nsISelection* aSel, nsIDocument* aDoc,
>-                        int16_t aClipboardID)
>+                        int16_t aClipboardID, bool aWithRubyAnnotation)
> {
>-  return SelectionCopyHelper(aSel, aDoc, true, aClipboardID,
>-                             nsIDocumentEncoder::SkipInvisibleContent,
>-                             nullptr);
>+  uint32_t flags = nsIDocumentEncoder::SkipInvisibleContent;
>+  if (aWithRubyAnnotation) {
>+    flags |= nsIDocumentEncoder::OutputRubyAnnotation;
>+  }
>+  return SelectionCopyHelper(aSel, aDoc, true, aClipboardID, flags, nullptr);
> }
> 
> nsresult
> nsCopySupport::GetTransferableForSelection(nsISelection* aSel,
>                                            nsIDocument* aDoc,
>                                            nsITransferable** aTransferable)
> {
>   return SelectionCopyHelper(aSel, aDoc, false, 0,
>@@ -686,17 +689,17 @@ nsCopySupport::FireClipboardEvent(int32_
>   if (doDefault) {
>     // get the data from the selection if any
>     bool isCollapsed;
>     sel->GetIsCollapsed(&isCollapsed);
>     if (isCollapsed) {
>       return false;
>     }
>     // call the copy code
>-    rv = HTMLCopy(sel, doc, aClipboardType);
>+    rv = HTMLCopy(sel, doc, aClipboardType, false);
>     if (NS_FAILED(rv)) {
>       return false;
>     }
>   } else if (clipboardData) {
>     // check to see if any data was put on the data transfer.
>     clipboardData->GetMozItemCount(&count);
>     if (count) {
>       nsCOMPtr<nsIClipboard> clipboard(do_GetService("@mozilla.org/widget/clipboard;1"));
>diff --git a/dom/base/nsCopySupport.h b/dom/base/nsCopySupport.h
>--- a/dom/base/nsCopySupport.h
>+++ b/dom/base/nsCopySupport.h
>@@ -18,17 +18,18 @@ class nsACString;
> class nsAString;
> class nsIPresShell;
> class nsILoadContext;
> 
> class nsCopySupport
> {
>   // class of static helper functions for copy support
>   public:
>-    static nsresult HTMLCopy(nsISelection *aSel, nsIDocument *aDoc, int16_t aClipboardID);
>+    static nsresult HTMLCopy(nsISelection *aSel, nsIDocument *aDoc,
>+                             int16_t aClipboardID, bool aWithRubyAnnotation);
>     static nsresult DoHooks(nsIDocument *aDoc, nsITransferable *aTrans,
>                             bool *aDoPutOnClipboard);
> 
>     // Get the selection, or entire document, in the format specified by the mime type
>     // (text/html or text/plain). If aSel is non-null, use it, otherwise get the entire
>     // doc.
>     static nsresult GetContents(const nsACString& aMimeType, uint32_t aFlags, nsISelection *aSel, nsIDocument *aDoc, nsAString& outdata);
>     
>diff --git a/layout/generic/nsSelection.cpp b/layout/generic/nsSelection.cpp
>--- a/layout/generic/nsSelection.cpp
>+++ b/layout/generic/nsSelection.cpp
>@@ -6127,10 +6127,11 @@ nsAutoCopyListener::NotifySelectionChang
>     /* clear X clipboard? */
>     return NS_OK;
>   }
> 
>   nsCOMPtr<nsIDocument> doc = do_QueryInterface(aDoc);
>   NS_ENSURE_TRUE(doc, NS_ERROR_FAILURE);
> 
>   // call the copy code
>-  return nsCopySupport::HTMLCopy(aSel, doc, nsIClipboard::kSelectionClipboard);
>-}
>+  return nsCopySupport::HTMLCopy(aSel, doc,
>+                                 nsIClipboard::kSelectionClipboard, false);
>+}
Attachment #8572423 - Flags: review?(bugs) → review+
Comment on attachment 8572426 [details] [diff] [review]
patch 4 - Copy with ruby when selection is inside ruby

>+IsInsideRuby(nsCOMPtr<nsINode> aNode)
Please don't use nsCOMPtr as an argument type.
That hides whether there is some addref/release happening.
Use nsINode*

>+static bool
>+IsSelectionInsideRuby(nsISelection* aSelection)
>+{
>+  int32_t rangeCount;
>+  nsresult rv = aSelection->GetRangeCount(&rangeCount);
>+  if (NS_FAILED(rv)) {
>+    return false;
>+  }
>+  for (auto i : MakeRange(rangeCount)) {
Hmm, perhaps the second time I see not horrible auto usage :)

>+    nsCOMPtr<nsIDOMRange> range;
>+    aSelection->GetRangeAt(i, getter_AddRefs(range));
>+    nsCOMPtr<nsIDOMNode> node;
>+    range->GetCommonAncestorContainer(getter_AddRefs(node));


>+    if (!IsInsideRuby(do_QueryInterface(node))) {
So you want nsCOMPtr<nsINode> n = do_QueryInterface(node);
and pass n;

r+

But I can't really say if this is a reasonable behavior. 
What does masayuki think of this?
Attachment #8572426 - Flags: review?(bugs) → review+
masayuki, do you think this patchset provides a reasonable behavior? See what is implemented here in comment 26.
Flags: needinfo?(masayuki)
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #29)
> masayuki, do you think this patchset provides a reasonable behavior? See
> what is implemented here in comment 26.

I think that it's reasonable in most cases. But I worry about the case rp/rt/rtc has different style than UA default style. E.g., all of them may be display: inline. In such cases, users may feel odd (but I guess this is not a scope of this bug, e.g., <pre> must have similar bug).

If I were you, I'd create a pref for disabling new feature. But I'm not sure if it's really necessary.
# I guess that nsDocumentEncoder is also used by Thunderbird to display HTML mails as plaintext mail. I also worry the side effect for that. Although, I have no idea of possible scenarios.

Anyway, a lot of testers can test this feature until beta release at least. So, we can back them out if there are some bug reports. I believer that there is a value in trying to test this feature.
Flags: needinfo?(masayuki)
I think it's reasonable to add a pref for that. I'll add it.
Add a pref so that we can switch this feature off if user doesn't really like it.
Assignee: nobody → quanxunzhen
Attachment #8571291 - Attachment is obsolete: true
Attachment #8574945 - Flags: review?(roc)
The leave-open keyword is there and there is no activity for 6 months.
:hsinyi, maybe it's time to close this bug?
Flags: needinfo?(htsai)
Flags: needinfo?(xidorn+moz)
The landed patches implemented the functionality behind a flag, but we might still want some UI to do that so I think it makes sense to leave it open. This is low priority, though.
Flags: needinfo?(xidorn+moz)
Priority: -- → P5
Thanks for the comment, Xidorn!
Flags: needinfo?(htsai)

The leave-open keyword is there and there is no activity for 6 months.
:hsinyi, maybe it's time to close this bug?

Flags: needinfo?(htsai)

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #39)

The leave-open keyword is there and there is no activity for 6 months.
:hsinyi, maybe it's time to close this bug?

no

Flags: needinfo?(htsai)
You need to log in before you can comment on or make changes to this bug.