819930 - API to emulate CSS Media Types

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Rob Campbell [:rc] (:robcee)]

We should provide a way to display pages rendered using different media types if available.

Not clear where would be the best place to put this control: The Style Editor, The Inspector, or a stand-alone option?

For an example of how Chrome's implemented it, see:

https://plus.google.com/u/0/115133653231679625609/posts/MgpioU84JPe

Comment 1

[Paul Rouget [:paul]]

We want a "option" menu in the toolbox toolbar (on the left of the tabs). Maybe there.

Comment 2

[Girish Sharma [:Optimizer]]

This might need its own option meni, as it will have a menu list from which users will choose. May be integrate this with Responsive Mode, as both are related ?

Comment 3

[Paul Rouget [:paul]]

We should not worry about the UI for now (we can expose this feature as a gcli-command to start).

Comment 4

[Graeme McCutcheon [:graememcc]]

Attachment: Part 1 - Add API for chrome callers to emulate CSS media types

Comment 5

[Graeme McCutcheon [:graememcc]]

Attachment: Part 2 - Add GCLI command

Holding off on r? until I've addressed any comments from part 1

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 775852 [details] [diff] [review]
Part 1 - Add API for chrome callers to emulate CSS media types

I'm not crazy about this feature, since media types are something we should be discouraging authors from using in the first place (and we should also ensure anything important covered by them is covered by media queries, if that's not the case already), and also since these media types are likely to be associated with other characteristics that this doesn't emulate.  Though I suppose giving authors the ability to test what happens with different media types helps them catch things that are unnecessarily type-specific.

As far as the patch goes -- please don't use |short| in C++ -- use int16_t or similar.  That said, uint8_t seems more sensible.  Or, actually... just make the API take a string argument.  Given that the caller has a string, the recipient wants a string, and strings make better API, it seems like you want the API to be string-based.  (I'd probably skip on the input validation, and just make mMediaEmulated by an nsCOMPtr<nsIAtom> or nsRefPtr<nsIAtom>.)

In nsPresContext.h, please put "bool mIsEmulatingMedia" next to the long list of 1-bit unsigned members, and make it a 1-bit unsigned like them.

Other than that, I guess this is fine, but I'd like to look at the revised patch.

Comment 7

[Graeme McCutcheon [:graememcc]]

Attachment: Part 1 - Add API for chrome callers to emulate CSS media types

David, thanks for that comment: I'd missed the obvious, strings make far more sense.

Comment 8

[Graeme McCutcheon [:graememcc]]

Attachment: Part 2 - Add GCLI command

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 776463 [details] [diff] [review]
Part 1 - Add API for chrome callers to emulate CSS media types

Drop the changes to content/base/src/nsGkAtomList.h; they're no longer 
needed.

I might prefer renaming resetMedium -> stopEmulatingMedium just to
make it clearer that what it resets is the emulation.

And I realize you probably also want to do the
EnumerateExternalResources bit that nsDocumentViewer::SetTextZoom (and
two other functions) do.  Otherwise you won't touch SVG resource 
documents, which you probably should for this feature.  (And, in fact,
it might not be a bad idea to write a version of CallChildren that does
external resources too, though as a separate patch, including fixing
the three existing users.)

Also, the test doesn't need SimpleTest.waitForExplicitFinish() and
SimpleTest.finish(), as far as I can see.  

I think you should also switch the test to use 
getComputedStyle(document.body, "background-color") rather than using
screenshots; it's much more efficient, and you can write more precise
tests (without the O(N^2) business).

r=dbaron with those changes

Comment 10

[Graeme McCutcheon [:graememcc]]

Attachment: Part 2 - Add GCLI command

Updated with the renaming suggested in comment 10

Comment 11

[Joe Walker [:jwalker]]

Comment on attachment 777158 [details] [diff] [review]
Part 2 - Add GCLI command

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

I like it - I think this could be really useful.
Thanks.

::: browser/devtools/commandline/BuiltinCommands.jsm
@@ +2207,5 @@
> +    description: gcli.lookup("mediaResetDesc"),
> +    manual: gcli.lookup("mediaEmulateManual"),
> +    exec: function(args, context) {
> +      let markupDocumentViewer = context.environment.chromeDocument.defaultView
> +                                        .gBrowser.markupDocumentViewer;

For what it's worth context.environment.chromeDocument.defaultView == context.environment.chromeWindow

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +1306,5 @@
> +
> +# LOCALIZATION NOTE (mediaResetManual) A fuller description of the
> +# 'media reset' command, displayed when the user asks for help on what it
> +# does.
> +mediaResetManual=Stop emulating a CSS media type

Our use of comments in l10n files isn't great, and I've a patch that's about to land that makes things much less repetitive. So can I suggest:

# LOCALIZATION NOTE (mediaDesc, mediaEmulateDesc, mediaEmulateManual,
# mediaEmulateType, mediaResetDesc, mediaResetManual) These strings describe
# the 'media' commands and all available parameters.
mediaDesc=CSS media type emulation
mediaEmulateDesc=Emulate a specified CSS media type
mediaEmulateManual=View the document as if rendered on a device supporting the given media type, with the relevant CSS rules applied.
mediaEmulateType=The media type to emulate
mediaResetDesc=Stop emulating a CSS media type
mediaResetManual=Stop emulating a CSS media type

Comment 12

[Graeme McCutcheon [:graememcc]]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #9)
> And, in fact, it might not be a bad idea to write a version of CallChildren that does
> external resources too

Filed bug 895328.

Comment 13

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66f4834afb70
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c42e1391bd2

There was an errant constant from an earlier iteration of part 1 still present in part 2, which I fixed on checkin.

Given comments 1-3, marking this leave-open for now in case DevTools want to iterate on exposing this further on the UI - if not please clear the whiteboard annotation.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/66f4834afb70
https://hg.mozilla.org/mozilla-central/rev/5c42e1391bd2

Comment 15

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Nice! As a matter of fact, I implemented something extremely similar, even
code-wise, in BlueGriffon. Please add a new API to nsIHTMLEditor so this
can be controlled from chrome JS above the editor w/o having to call
execCommand. Thanks!

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Let's call this one done, since we did land an API here in Firefox 25.

I've filed bug 1202711 for exposing a UI to control it in responsive design.