1130891 - Add option to exclude ruby annotation from plain text when copied

Status: RESOLVED FIXED

(Core :: DOM: Selection, enhancement, P5)

Description

[Alice0775 White]

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

Comment 1

[Mats Palmgren (inactive)]

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)

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

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?

Comment 3

[Mats Palmgren (inactive)]

(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?

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

(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

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

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?

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> 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

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - add rb/rtc to atom list

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - add flag in document encoder to control

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - add context menu item

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

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 13

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

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.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

(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?

Comment 15

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

Yeah, a bool param should be fine.

Comment 16

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

(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 17

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

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.

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 19

[Mats Palmgren (inactive)]

> 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. ;-)

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

(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 21

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 23

[Jet Villegas (inactive)]

(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.

Comment 24

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

(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.

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - Add param to HTMLCopy

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 4 - Copy with ruby when selection is inside ruby

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.

Comment 27

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

Comment on attachment 8572423 [details] [diff] [review]
patch 3 - Add param to HTMLCopy

># HG changeset patch
># User Xidorn Quan <[e-mail]>
># 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);
>+}

Comment 28

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

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?

Comment 29

[Xidorn Quan [:xidorn] UTC+10]

masayuki, do you think this patchset provides a reasonable behavior? See what is implemented here in comment 26.

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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.

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

I think it's reasonable to add a pref for that. I'll add it.

Comment 32

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - add flag in document encoder to control

Add a pref so that we can switch this feature off if user doesn't really like it.

Comment 33

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1472e4b3cf53

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05e23665e63e
https://hg.mozilla.org/integration/mozilla-inbound/rev/90727c00a60a
https://hg.mozilla.org/integration/mozilla-inbound/rev/209982f6fccb
https://hg.mozilla.org/integration/mozilla-inbound/rev/94d5173fa1cd

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/05e23665e63e
https://hg.mozilla.org/mozilla-central/rev/90727c00a60a
https://hg.mozilla.org/mozilla-central/rev/209982f6fccb
https://hg.mozilla.org/mozilla-central/rev/94d5173fa1cd

Comment 36

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 37

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 38

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Thanks for the comment, Xidorn!

Comment 39

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 40

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

(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

Comment 41

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 42

[Xidorn Quan [:xidorn] UTC+10]

no

Comment 43

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 44

[Xidorn Quan [:xidorn] UTC+10]

no

Comment 45

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 46

[Jens Stutte [:jstutte]]

Mirko, can you check if there is something left to do here?

Comment 47

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Mirko, can you check if there is something left to do here?

According to #c37, there is.

Madhava (UX person) isn't working for Mozilla anymore. So we should ni? someone else from UX. Unfortunately, I don't know whom else to ni?. annevk: do you know someone?

Comment 48

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Btw., it seems parts of the code were landed, but without test. It seems flipping the pref ("converter.html2txt.always_include_ruby") doesn't change the plaintext. Hence, what was landed might have been broken.

Comment 49

[Anne (:annevk)]

So assuming we fix what was broken the question to UX would be how to expose to the user the ability to copy ruby text along with the ruby base when copying plain text (and not HTML)? (See https://en.wikipedia.org/wiki/Ruby_character for context.) It seems we have at least these options:

1. Not offer anything. (Status quo I assume?)
2. Copy the ruby text only if only the ruby container was selected. (Suggested by roc above.)
3. Offer a "Copy with ruby text" or equivalent contextmenu item. (It seems this would also require additional work to determine when it would be appropriate to display that if I'm reading the patches correctly.)

Markus, is this something you or your team could give guidance on?

Comment 50

[Xidorn Quan [:xidorn] UTC+10]

I can have a look at why it was broken, and possibly fix it and add tests.

Comment 51

[Markus Jaritz [:markus] (UX)]

(In reply to Anne (:annevk) from comment #49)

Markus, is this something you or your team could give guidance on?

Hi Anne, sorry we can not offer to fully think through this if this is not part of our bigger initiatives as we are very resource constrained.
What I can do is leave you my thoughts and please don't feel blocked by us from moving forward with what you deem right.

So assuming we fix what was broken the question to UX would be how to expose to the user the ability to copy ruby text along with the ruby base when copying plain text (and not HTML)? (See https://en.wikipedia.org/wiki/Ruby_character for context.) It seems we have at least these options:

1. Not offer anything. (Status quo I assume?)

we do quite a lot currently

• copy text with ruby in parenthesis in the wikipedia example. (more differentiation then Chrome or Edge make)
• copy text without ruby when selecting a full ruby block by tripple-clicking
• search for text without ruby when using context menu "search with..." entry
  this might explain why we see a surprisingly high usage of that search access point in Japan - but never had an explanation for that.

2. Copy the ruby text only if only the ruby container was selected. (Suggested by roc above.)

Looks like this is implemented already but very hard to discover.
As I don't know how often or widely this is used or what peoples needs are it is hard to judge if this should be more accessible in a way like suggested in 3.

3. Offer a "Copy with ruby text" or equivalent contextmenu item. (It seems this would also require additional work to determine when it would be appropriate to display that if I'm reading the patches correctly.)

Comment 52

[Xidorn Quan [:xidorn] UTC+10]

Btw., it seems parts of the code were landed, but without test. It seems flipping the pref ("converter.html2txt.always_include_ruby") doesn't change the plaintext. Hence, what was landed might have been broken.

I just tested it and it doesn't seem to be broken. It still seems to work as expected that when the pref is set, ruby text is always copied. But I'll see if I can add a test for it.

Comment 53

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1130891 part 5 - Add test for the copy behavior. r?smaug

Comment 54

[Xidorn Quan [:xidorn] UTC+10]

That's being said, I think the current behavior isn't very helpful. I would probably want to change it to something like, if all text nodes being selected are in ruby, then copy ruby annotation, because it's not always easy (actually it's quite hard) to select the whole ruby while leaving the selection position just fall inside ruby.

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ddecf8b9571
part 5 - Add test for the copy behavior. r=smaug

Comment 56

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/1ddecf8b9571

Comment 57

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 58

[Jens Stutte [:jstutte]]

I read from several comments that there is still work to do here. But I have a hard time to understand the state here, it feels a bit as if we might rather want to file a new, better scoped bug and close this one?

Comment 59

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jens Stutte [:jstutte] from comment #58)

I read from several comments that there is still work to do here. But I have a hard time to understand the state here, it feels a bit as if we might rather want to file a new, better scoped bug and close this one?

Well, Xidorn, how about to file a new file for follow up things?

Comment 60

[Xidorn Quan [:xidorn] UTC+10]

Sounds good. Filed a new bug 1722215.