Enable plaintext serializer to output non-text content as placeholder

RESOLVED FIXED in Firefox 25, Firefox OS v1.1hd

Status

()

Core
Serializers
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: yxl, Assigned: yxl)

Tracking

unspecified
mozilla25
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:leo+, firefox23 wontfix, firefox24 wontfix, firefox25 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

We need a plaintext serializer to output non-textual element as placeholder character.
For example "<span>good<img src="xxx.png">day</span" will be serialized to "good[placeholder]day", but not "goodday" without placeholder.

The main user case is below.

Firefox OS keyboard treats all text field as plaintext and depends on the plaintext serializer to get the text content of a contentEditable element. When user is editing a rich text field, there is no way for the keyboard to aware of an image(with empty alt and title text) or other non-textual element), as these non-textual elements will be outputted as empty string. For example "go<img>od" will be outputted as "good". It makes troubles for the keyboard. See bug 883129 for details.
Created attachment 771735 [details] [diff] [review]
Output non-text content as the placeholder character SUM

The patch adds the flag OutputNonTextContentAsPlaceholder to nsIDocumentEncoder, which can be only used for serializing to plaintext. If the flag is set, all non-textual elements will be outputted as a placeholder character SUB('\x1A').

Hi Henry, could you help review the patch or recommend someone else? Thanks in advance.
Attachment #771735 - Flags: review?(hsivonen)
Created attachment 771736 [details] [diff] [review]
test case
Attachment #771736 - Flags: review?(hsivonen)
Comment on attachment 771735 [details] [diff] [review]
Output non-text content as the placeholder character SUM

Why U+001A? Why not U+FFFC (OBJECT REPLACEMENT CHARACTER), which seems to exist for this purpose?
Comment on attachment 771736 [details] [diff] [review]
test case

>+		file_placeholder.png \

Does this file actually need to exist in order to test the serializer?

>+    netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");

You are not supposed to use UniversalXPConnect in new tests. Please either use SpecialPowers somehow (not sure if SpecialPowers can be used here) or write a mochitest-chrome test instead of a mochitest-plain test.

>+    out = toPlaintext("case1");
>+    expected = "This is a button. Hello!";
>+    is(out, expected, "test with <button>");

Why not just
is(toPlaintext("case1"), "This is a button. Hello!", "test with <button>");
?
Attachment #771736 - Flags: review?(hsivonen) → review-
(In reply to Henri Sivonen (:hsivonen) from comment #3)
> Comment on attachment 771735 [details] [diff] [review]
> Output non-text content as the placeholder character SUM
> 
> Why U+001A? Why not U+FFFC (OBJECT REPLACEMENT CHARACTER), which seems to
> exist for this purpose?

It's really a good idea to use U+FFFC (OBJECT REPLACEMENT CHARACTER) instead. I can't find any character better.
Created attachment 771999 [details] [diff] [review]
Output non-text content as the placeholder character U+FFFC (OBJECT REPLACEMENT CHARACTER).
Attachment #771735 - Attachment is obsolete: true
Attachment #771735 - Flags: review?(hsivonen)
Attachment #771999 - Flags: review?(hsivonen)
Created attachment 772098 [details] [diff] [review]
test case

(In reply to Henri Sivonen (:hsivonen) from comment #4)
> Comment on attachment 771736 [details] [diff] [review]
> test case
> 
> >+		file_placeholder.png \
> 
> Does this file actually need to exist in order to test the serializer?
> 

No, it doesn't and is removed from this patch.

> >+    netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> 
> You are not supposed to use UniversalXPConnect in new tests. Please either
> use SpecialPowers somehow (not sure if SpecialPowers can be used here) or
> write a mochitest-chrome test instead of a mochitest-plain test.

Use SpecialPowers instead.

> 
> >+    out = toPlaintext("case1");
> >+    expected = "This is a button. Hello!";
> >+    is(out, expected, "test with <button>");
> 
> Why not just
> is(toPlaintext("case1"), "This is a button. Hello!", "test with <button>");
> ?

done.
Attachment #771736 - Attachment is obsolete: true
Attachment #772098 - Flags: review?(hsivonen)
Attachment #771999 - Flags: review?(hsivonen) → review+
Attachment #772098 - Flags: review?(hsivonen) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7b1fd1f4f7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/df4cc5d63c7d
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b7b1fd1f4f7f
https://hg.mozilla.org/mozilla-central/rev/df4cc5d63c7d
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(Assignee)

Updated

4 years ago
Blocks: 895239
This bug should be fixed before the leo+ bug 895239.
blocking-b2g: --- → leo?
This bug should be fixed in b2g18 branch as well.
Moving to leo+ Blocks a blocker: bug 895239
blocking-b2g: leo? → leo+
https://hg.mozilla.org/releases/mozilla-b2g18/rev/40ab2e3076f1
https://hg.mozilla.org/releases/mozilla-b2g18/rev/58a4e9af311f
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-b2g-v1.1hd: --- → affected
status-firefox23: --- → wontfix
status-firefox24: --- → wontfix
status-firefox25: --- → fixed
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/40ab2e3076f1
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/58a4e9af311f
status-b2g-v1.1hd: affected → fixed
You need to log in before you can comment on or make changes to this bug.