Bug 183708 - Web Inspector: Elements: "Force Print Media Styles" should not persist across Web Inspector sessions
Summary: Web Inspector: Elements: "Force Print Media Styles" should not persist across...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-16 14:33 PDT by Nikita Vasilyev
Modified: 2018-03-16 16:43 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.35 KB, patch)
2018-03-16 14:40 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
[Animated GIF] With patch applied (259.18 KB, image/gif)
2018-03-16 14:47 PDT, Nikita Vasilyev
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2018-03-16 14:33:51 PDT
Several people accidentally enabled print stylesheets by clicking the printer icon in the Elements tab
and didn't know what happened and how to change it back.

Don't save "Force Print Media Styles" state to WI localStorage,
so re-opening Web Inspector should always have "Force Print Media Styles" disabled.
Comment 1 Nikita Vasilyev 2018-03-16 14:34:03 PDT
<rdar://problem/36452183>
Comment 2 Nikita Vasilyev 2018-03-16 14:40:23 PDT
Created attachment 335971 [details]
Patch
Comment 3 Nikita Vasilyev 2018-03-16 14:47:04 PDT
Created attachment 335973 [details]
[Animated GIF] With patch applied
Comment 4 Matt Baker 2018-03-16 14:59:29 PDT
Comment on attachment 335971 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=335971&action=review

r=me

> Source/WebInspectorUI/UserInterface/Base/Main.js:182
> +    this.printStylesEnabled = false;

This could be a static property of DOMTreeContentView, since it's now the only place it's used. But I'll leave it up to you.
Comment 5 Nikita Vasilyev 2018-03-16 15:09:13 PDT
Comment on attachment 335971 [details]
Patch

(In reply to Matt Baker from comment #4)
> Comment on attachment 335971 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335971&action=review
> 
> r=me
> 
> > Source/WebInspectorUI/UserInterface/Base/Main.js:182
> > +    this.printStylesEnabled = false;
> 
> This could be a static property of DOMTreeContentView, since it's now the
> only place it's used. But I'll leave it up to you.

Consider the following case:
1. Open a wikipedia article, say https://en.wikipedia.org/wiki/Metropolitan_statistical_area
2. Click "Force Print Media Styles"
3. Navigate to a different article, such as https://en.wikipedia.org/wiki/U.S._state

Expected:
Print styles stay enabled.

If I make printStylesEnabled a private property of DOMTreeContentView, print styles would reset back to disabled.
Comment 6 Matt Baker 2018-03-16 15:16:36 PDT
Comment on attachment 335971 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=335971&action=review

>>> Source/WebInspectorUI/UserInterface/Base/Main.js:182
>>> +    this.printStylesEnabled = false;
>> 
>> This could be a static property of DOMTreeContentView, since it's now the only place it's used. But I'll leave it up to you.
> 
> Consider the following case:
> 1. Open a wikipedia article, say https://en.wikipedia.org/wiki/Metropolitan_statistical_area
> 2. Click "Force Print Media Styles"
> 3. Navigate to a different article, such as https://en.wikipedia.org/wiki/U.S._state
> 
> Expected:
> Print styles stay enabled.
> 
> If I make printStylesEnabled a private property of DOMTreeContentView, print styles would reset back to disabled.

I suggested making it a static (class) property. Using a private property would also cause print styles to be reset when removing and adding the Element tab. It's fine as-is in Main.js.
Comment 7 Nikita Vasilyev 2018-03-16 15:18:48 PDT
Whoops, I misread it. I still prefer to have a state variable in Main.js.
Comment 8 WebKit Commit Bot 2018-03-16 15:33:51 PDT
Comment on attachment 335971 [details]
Patch

Rejecting attachment 335971 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 335971, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
org/git/WebKit
   8fd1f9b8800..3238fbbc75c  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 229683 = 8fd1f9b880041cdae470c6c642bce201f506e686
r229684 = 3238fbbc75c4392730fb9fc646e5f497d8c9b2b7
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/6987624
Comment 9 Nikita Vasilyev 2018-03-16 16:19:04 PDT
Comment on attachment 335971 [details]
Patch

Unrelated test failure.
Comment 10 WebKit Commit Bot 2018-03-16 16:43:46 PDT
Comment on attachment 335971 [details]
Patch

Clearing flags on attachment: 335971

Committed r229686: <https://trac.webkit.org/changeset/229686>
Comment 11 WebKit Commit Bot 2018-03-16 16:43:47 PDT
All reviewed patches have been landed.  Closing bug.