Support sharing HTML so we can share with more apps in Windows 8

RESOLVED FIXED

Status

()

RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [metro-it2][LOE:1][completed-elm])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
We currently support sharing only what's selected but it is text only.
This should also support HTML text selections.
(Assignee)

Comment 1

6 years ago
I really like this feature by the way for sharing to the mail app and to evernote. I did most of this over the holidays, just finished up today.
(Assignee)

Comment 2

6 years ago
Created attachment 698069 [details] [diff] [review]
Patch v1
Assignee: nobody → netzen
Attachment #698069 - Flags: review?(jmathies)
(Assignee)

Comment 3

6 years ago
This line is for parity with IE10:
props->put_Description(HStringReference(url.BeginReading()).Get());

It shows up on the sharing panes better for a bunch of apps when you do that.

Comment 4

6 years ago
Comment on attachment 698069 [details] [diff] [review]
Patch v1

On the winrt bits, r+. Forwarding the review on the logic in _expandURLs to ehsan.
Attachment #698069 - Flags: review?(jmathies)
Attachment #698069 - Flags: review?(ehsan)
Attachment #698069 - Flags: review+
(Assignee)

Comment 5

6 years ago
ehsan let me know if you can think of other important tags you think we should expand as well assuming the expanding code is OK.

Comment 6

6 years ago
Comment on attachment 698069 [details] [diff] [review]
Patch v1

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

::: widget/windows/winrt/MetroUIUtils.js
@@ +97,5 @@
> +      } else {
> +        this._expandURLs(n.children[i]);
> +      }
> +    }
> +  },

Stealing ideas from <http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsTreeSanitizer.cpp#289>, you want to handle things like:

* form.action
* link.href
* img.longdesc

etc.
Attachment #698069 - Flags: review?(ehsan) → review-
(Assignee)

Comment 7

6 years ago
Created attachment 698819 [details] [diff] [review]
Patch v2

expanded support for resolving URLs. 
Also changed to getAttribute/setAttribute because the previous trick no longer worked for some attributes.
Attachment #698069 - Attachment is obsolete: true
Attachment #698819 - Flags: review?(ehsan)
(Assignee)

Comment 8

6 years ago
Comment on attachment 698819 [details] [diff] [review]
Patch v2

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

Actually let me replace all those toLower() calls with a single one.
Attachment #698819 - Flags: review?(ehsan)
(Assignee)

Comment 9

6 years ago
Created attachment 698823 [details] [diff] [review]
Patch v3.
Attachment #698819 - Attachment is obsolete: true
Attachment #698823 - Flags: review?(ehsan)

Comment 10

6 years ago
Comment on attachment 698823 [details] [diff] [review]
Patch v3.

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

Sorry for being pedantic, but this code can be made less horrible if you define a data table like below and just iterate over it.  r=me with that.

{
  'a': 'href',
  'img': ['src', 'longdesc'],
  ...
}
Attachment #698823 - Flags: review?(ehsan) → review+
(Assignee)

Comment 11

6 years ago
ok sounds good.
(Assignee)

Comment 12

6 years ago
Implemented comments as per comment 10
https://hg.mozilla.org/projects/elm/rev/6085fdcdfe88
Whiteboard: [metro-it2][LOE:1] → [metro-it2][LOE:1][completed-elm]

Comment 13

6 years ago
(In reply to comment #12)
> Implemented comments as per comment 10
> https://hg.mozilla.org/projects/elm/rev/6085fdcdfe88

Nit on https://hg.mozilla.org/projects/elm/rev/6085fdcdfe88#l3.19: you should probably use const.
(Assignee)

Updated

6 years ago
Duplicate of this bug: 823961

Updated

6 years ago
No longer blocks: 801131
(Assignee)

Updated

6 years ago
Blocks: 833123
(Assignee)

Comment 16

6 years ago
This was part of the elm -> m-c migration.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.