Bug 140580 - Web Inspector: Tweak the styles on the Console
Summary: Web Inspector: Tweak the styles on the Console
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-01-16 17:53 PST by Timothy Hatcher
Modified: 2017-06-04 12:37 PDT (History)
9 users (show)

See Also:


Attachments
WIP (15.69 KB, patch)
2015-01-27 14:17 PST, Nikita Vasilyev
timothy: review-
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
[Image] WIP (69.09 KB, image/png)
2015-01-27 14:18 PST, Nikita Vasilyev
no flags Details
WIP (18.53 KB, patch)
2015-02-01 21:59 PST, Nikita Vasilyev
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch (26.07 KB, patch)
2015-02-14 21:05 PST, Nikita Vasilyev
timothy: review+
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch (28.15 KB, patch)
2015-02-16 18:37 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2015-01-16 17:53:11 PST
The console feels dated with monospace font for everything.

Some tweaks we should make:

* Use Menlo only for code. Use the system font for everything else.
* Syntax highlight previous command, don't force them to be all blue text.
* Color the background for errors and warnings.
* Bump the fond and margins a bit.
* Use less disclosure triangles for things. Especially for errors and stack traces.

These tweaks are shown in a static demo here: http://timothy.hatcher.name/console/
Comment 1 Radar WebKit Bug Importer 2015-01-16 17:53:37 PST
<rdar://problem/19507400>
Comment 2 Timothy Hatcher 2015-01-16 19:49:58 PST
See bug 100302 for Nikita's old attempt at some of this, that was rejected by Chrome dev.

Lets prevail!
Comment 3 Nikita Vasilyev 2015-01-27 14:17:08 PST
Created attachment 245468 [details]
WIP
Comment 4 Nikita Vasilyev 2015-01-27 14:18:01 PST
Created attachment 245469 [details]
[Image] WIP

Screenshot of the WIP patch applied.
Comment 5 Nikita Vasilyev 2015-01-27 14:22:05 PST
I’m aware of the following problems:
– vertical misalignment of Menlo text when it’s on the same line with non-monospace Helvetica;
– smaller font size for console.error messages;
Comment 6 Nikita Vasilyev 2015-01-29 19:42:00 PST
How are we going to display objects for

    console.log("Document: %o. Window: %o", document, window)

?

This is how it is now: https://cldup.com/l76xpiPkSD.gif

The current same line object expansion is hideous. There are a few options to make it better:

1. Don’t allow expansion for the %o objects, just show the previews. If a person wants an object to be expandable, they should use a separate console.log (without %o) to log it on a separate line.

2. Another option would be "tabs" – only one object could be expanded at the time. E.g.  expanding "window" would collapse document, and vice versa. The expanded object could be displayed one line below the the "Document: {...} . Window: {...}" line.

WDYT?
Comment 7 Nikita Vasilyev 2015-01-29 20:07:04 PST
Firefox DevTools and Firebug expand objects on the sidebar:

https://cldup.com/R8riS5JrqF.gif

I have to admit I actually like it.

There is no indication which object is expanded; it could be improved it by highlighting it.

Also, notice non-monospace sans-serif font.
Comment 8 Joseph Pecoraro 2015-01-29 22:10:37 PST
(In reply to comment #7)
> Firefox DevTools and Firebug expand objects on the sidebar:
> 
> https://cldup.com/R8riS5JrqF.gif
> 
> I have to admit I actually like it.

They do this for everything, not just %o.

It is kind of cool, but I think it is much harder to work with if you want to view multiple objects. In Web Inspector's console, you can expand a few objects see everything, and scroll quickly between them. With the sidebar that is harder. They do have a neat "diff" thing if you click between two similar objects.


(In reply to comment #6)
> How are we going to display objects for
> 
>     console.log("Document: %o. Window: %o", document, window)
> 
> ?
> 
> This is how it is now: https://cldup.com/l76xpiPkSD.gif

I wonder how many people use %o? Personally I don't.

Given how poor the experience can be, and how much better (I think) the console.log("...", document, window) is, maybe we shouldn't invest too much into %o.


> The current same line object expansion is hideous. There are a few options
> to make it better:
> 
> 1. Don’t allow expansion for the %o objects, just show the previews. If a
> person wants an object to be expandable, they should use a separate
> console.log (without %o) to log it on a separate line.

There could be a preview, and still log below if the message itself is expanded.

Honestly, I've been interested in a %j that will JSON.stringify an object. Maybe that is what %o could do, and fallback to something like a preview when not possible or the string is too long.



> 2. Another option would be "tabs" – only one object could be expanded at the
> time. E.g.  expanding "window" would collapse document, and vice versa. The
> expanded object could be displayed one line below the the "Document: {...} .
> Window: {...}" line.

Hmm, neat idea.  Probably not worth a special case UI for this unless we think we could re-use that UI. Its like a mix between the console now and the FF approach.
Comment 9 Nikita Vasilyev 2015-01-29 22:47:45 PST
(In reply to comment #8)
> (In reply to comment #7)
> > Firefox DevTools and Firebug expand objects on the sidebar:
> > 
> > https://cldup.com/R8riS5JrqF.gif
> > 
> > I have to admit I actually like it.
> 
> They do this for everything, not just %o.
> 
> It is kind of cool, but I think it is much harder to work with if you want
> to view multiple objects. In Web Inspector's console, you can expand a few
> objects see everything, and scroll quickly between them. With the sidebar
> that is harder. They do have a neat "diff" thing if you click between two
> similar objects.
> 
> 
> (In reply to comment #6)
> > How are we going to display objects for
> > 
> >     console.log("Document: %o. Window: %o", document, window)
> > 
> > ?
> > 
> > This is how it is now: https://cldup.com/l76xpiPkSD.gif
> 
> I wonder how many people use %o? Personally I don't.
> 
> Given how poor the experience can be, and how much better (I think) the
> console.log("...", document, window) is, maybe we shouldn't invest too much
> into %o.

I don't use %o either and from the top of my head I can't remember any library/framework that uses it.

I want to use `line-height: baseline` for console messages, it would nicely vertically align monospace and non-monospace text.

`line-height: baseline`, however, breaks expanded same line objects: https://cldup.com/cfhGu5CPYH-2000x2000.png.
Since we are moving them on the separate line anyway, I was wondering if we can safely use `line-height: baseline` now.

%O is one of the edge cases here.


> 
> > The current same line object expansion is hideous. There are a few options
> > to make it better:
> > 
> > 1. Don’t allow expansion for the %o objects, just show the previews. If a
> > person wants an object to be expandable, they should use a separate
> > console.log (without %o) to log it on a separate line.
> 
> There could be a preview, and still log below if the message itself is
> expanded.

Say, we expanded one object. When we expand a second one, what should happen? Should it show before the first one? After?


> 
> Honestly, I've been interested in a %j that will JSON.stringify an object.
> Maybe that is what %o could do, and fallback to something like a preview
> when not possible or the string is too long.
> 
> 
> 
> > 2. Another option would be "tabs" – only one object could be expanded at the
> > time. E.g.  expanding "window" would collapse document, and vice versa. The
> > expanded object could be displayed one line below the the "Document: {...} .
> > Window: {...}" line.
> 
> Hmm, neat idea.  Probably not worth a special case UI for this unless we
> think we could re-use that UI. Its like a mix between the console now and
> the FF approach.
Comment 10 Timothy Hatcher 2015-01-30 21:16:05 PST
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Firefox DevTools and Firebug expand objects on the sidebar:
> > > 
> > > https://cldup.com/R8riS5JrqF.gif
> > > 
> > > I have to admit I actually like it.
> > 
> > They do this for everything, not just %o.
> > 
> > It is kind of cool, but I think it is much harder to work with if you want
> > to view multiple objects. In Web Inspector's console, you can expand a few
> > objects see everything, and scroll quickly between them. With the sidebar
> > that is harder. They do have a neat "diff" thing if you click between two
> > similar objects.
> > 
> > 
> > (In reply to comment #6)
> > > How are we going to display objects for
> > > 
> > >     console.log("Document: %o. Window: %o", document, window)
> > > 
> > > ?
> > > 
> > > This is how it is now: https://cldup.com/l76xpiPkSD.gif
> > 
> > I wonder how many people use %o? Personally I don't.
> > 
> > Given how poor the experience can be, and how much better (I think) the
> > console.log("...", document, window) is, maybe we shouldn't invest too much
> > into %o.
> 
> I don't use %o either and from the top of my head I can't remember any
> library/framework that uses it.
> 
> I want to use `line-height: baseline` for console messages, it would nicely
> vertically align monospace and non-monospace text.
> 
> `line-height: baseline`, however, breaks expanded same line objects:
> https://cldup.com/cfhGu5CPYH-2000x2000.png.
> Since we are moving them on the separate line anyway, I was wondering if we
> can safely use `line-height: baseline` now.
> 
> %O is one of the edge cases here.

I agree, %o is an edge case. I would just inline previews that are not expandable, and show the expandable objects "below the fold" (when expanding the log message). Clicking the preview could expand the log and jump to / highlight the object row too.

> > > The current same line object expansion is hideous. There are a few options
> > > to make it better:
> > > 
> > > 1. Don’t allow expansion for the %o objects, just show the previews. If a
> > > person wants an object to be expandable, they should use a separate
> > > console.log (without %o) to log it on a separate line.
> > 
> > There could be a preview, and still log below if the message itself is
> > expanded.
> 
> Say, we expanded one object. When we expand a second one, what should
> happen? Should it show before the first one? After?

You can expand multiple objects in the proposed row design. Each object is on a bulleted row under the log message. That way there is never weird inline flowing of expanded objects. Inline is reserves for primitives or previews only.
 
> > Honestly, I've been interested in a %j that will JSON.stringify an object.
> > Maybe that is what %o could do, and fallback to something like a preview
> > when not possible or the string is too long.

Why not just a preview? It is practically JSON. (We should as a context menu item for objects "Copy as JSON".)

> > > 2. Another option would be "tabs" – only one object could be expanded at the
> > > time. E.g.  expanding "window" would collapse document, and vice versa. The
> > > expanded object could be displayed one line below the the "Document: {...} .
> > > Window: {...}" line.
> > 
> > Hmm, neat idea.  Probably not worth a special case UI for this unless we
> > think we could re-use that UI. Its like a mix between the console now and
> > the FF approach.

Agreed.
Comment 11 Timothy Hatcher 2015-01-30 21:17:08 PST
> Since we are moving them on the separate line anyway, I was wondering if we
> can safely use `line-height: baseline` now.

We should use baseline alignment now.
Comment 12 Timothy Hatcher 2015-01-30 21:23:44 PST
(In reply to comment #8)
> (In reply to comment #7)
> > Firefox DevTools and Firebug expand objects on the sidebar:
> > 
> > https://cldup.com/R8riS5JrqF.gif
> > 
> > I have to admit I actually like it.
> 
> They do this for everything, not just %o.
> 
> It is kind of cool, but I think it is much harder to work with if you want
> to view multiple objects. In Web Inspector's console, you can expand a few
> objects see everything, and scroll quickly between them. With the sidebar
> that is harder. They do have a neat "diff" thing if you click between two
> similar objects.

I don't like the FireFox approach (which his a Firebug-ism). The sidebar has no context  for where you came from. With that said…

It could fit into our Details Sidebar design. The Details Sidebar is all about represented objects that are shown in the view. Is an object can be selected in the console, we could push a navigation item and show a Details Sidebar for the object tree. Similar to the Scope Chain Details Sidebar.

I'm still not sure that is the best way to view objects. It wouldn't be much different that using a popover at that point, which I think we all agreed wasn't good. How is a sidebar better?
Comment 13 Timothy Hatcher 2015-01-30 21:26:00 PST
But maybe our popovers just needs some work. They are a little flaky.
Comment 14 Timothy Hatcher 2015-01-30 21:30:19 PST
Comment on attachment 245468 [details]
WIP

Overall the patch looks good. We should still proceed with a redo here. The console code is mired from years of abuse at the hands of others. It is time to start fresh and not spend much time cleaning up this code. The DetailsSection*.js files are a good example of how this could be structured with the "row" subclasses providing concrete DOM views for different data.
Comment 15 Nikita Vasilyev 2015-01-31 04:20:44 PST
(In reply to comment #6)
> How are we going to display objects for
> 
>     console.log("Document: %o. Window: %o", document, window)
> 
> ?
> 
> This is how it is now: https://cldup.com/l76xpiPkSD.gif
> 
> The current same line object expansion is hideous. There are a few options
> to make it better:
> 
> 1. Don’t allow expansion for the %o objects, just show the previews. If a
> person wants an object to be expandable, they should use a separate
> console.log (without %o) to log it on a separate line.
> 
> 2. Another option would be "tabs" – only one object could be expanded at the
> time. E.g.  expanding "window" would collapse document, and vice versa. The
> expanded object could be displayed one line below the the "Document: {...} .
> Window: {...}" line.
> 
> WDYT?

I re-read my comment and realized that it isn’t clear.

I’m suggesting to keep the objects previews inline with the text, the same way it’s currently done. For example:

-> Document: {alinkColor: "", ...}. Window: {Infinity: Infinity, ...}

However, I don’t want the preview to be expandable. Please let me know if we agree up to this line.
Comment 16 Nikita Vasilyev 2015-01-31 04:27:32 PST
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #7)
> > > > The current same line object expansion is hideous. There are a few options
> > > > to make it better:
> > > > 
> > > > 1. Don’t allow expansion for the %o objects, just show the previews. If a
> > > > person wants an object to be expandable, they should use a separate
> > > > console.log (without %o) to log it on a separate line.
> > > 
> > > There could be a preview, and still log below if the message itself is
> > > expanded.
> > 
> > Say, we expanded one object. When we expand a second one, what should
> > happen? Should it show before the first one? After?
> 
> You can expand multiple objects in the proposed row design. Each object is
> on a bulleted row under the log message. That way there is never weird
> inline flowing of expanded objects. Inline is reserves for primitives or
> previews only.

So, are you suggesting something like this:

>> console.log("Document: %o. Window: %o", document, window)
-> Document: {alinkColor: "", ...}. Window: {Infinity: Infinity, ...}
• ► {alinkColor: "", ...}
• ► {Infinity: Infinity, ...}

E.g. there are non-expandable inline previews and objects are duplicated on each row below the message.

?
Comment 17 Nikita Vasilyev 2015-01-31 04:35:36 PST
(In reply to comment #12)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Firefox DevTools and Firebug expand objects on the sidebar:
> > > 
> > > https://cldup.com/R8riS5JrqF.gif
> > > 
> > > I have to admit I actually like it.
> > 
> > They do this for everything, not just %o.
> > 
> > It is kind of cool, but I think it is much harder to work with if you want
> > to view multiple objects. In Web Inspector's console, you can expand a few
> > objects see everything, and scroll quickly between them. With the sidebar
> > that is harder. They do have a neat "diff" thing if you click between two
> > similar objects.
> 
> I don't like the FireFox approach (which his a Firebug-ism). The sidebar has
> no context  for where you came from. With that said…
> 
> It could fit into our Details Sidebar design. The Details Sidebar is all
> about represented objects that are shown in the view. Is an object can be
> selected in the console, we could push a navigation item and show a Details
> Sidebar for the object tree. Similar to the Scope Chain Details Sidebar.

I wasn't clear here again.

I like how Firebug/Firefox sidebar approach handles this particular edge case, but I don’t like it overall. I’m not suggesting to use it.

> I'm still not sure that is the best way to view objects. It wouldn't be much
> different that using a popover at that point, which I think we all agreed
> wasn't good. How is a sidebar better?

One thing: it doesn't cover existing content.

But again, I’m not suggesting to use it. I agree it has no context for where you came from.
Comment 18 Timothy Hatcher 2015-01-31 05:52:14 PST
(In reply to comment #16)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (In reply to comment #8)
> > > > (In reply to comment #7)
> > > > > The current same line object expansion is hideous. There are a few options
> > > > > to make it better:
> > > > > 
> > > > > 1. Don’t allow expansion for the %o objects, just show the previews. If a
> > > > > person wants an object to be expandable, they should use a separate
> > > > > console.log (without %o) to log it on a separate line.
> > > > 
> > > > There could be a preview, and still log below if the message itself is
> > > > expanded.
> > > 
> > > Say, we expanded one object. When we expand a second one, what should
> > > happen? Should it show before the first one? After?
> > 
> > You can expand multiple objects in the proposed row design. Each object is
> > on a bulleted row under the log message. That way there is never weird
> > inline flowing of expanded objects. Inline is reserves for primitives or
> > previews only.
> 
> So, are you suggesting something like this:
> 
> >> console.log("Document: %o. Window: %o", document, window)
> -> Document: {alinkColor: "", ...}. Window: {Infinity: Infinity, ...}
> • ► {alinkColor: "", ...}
> • ► {Infinity: Infinity, ...}
> 
> E.g. there are non-expandable inline previews and objects are duplicated on
> each row below the message.

Yes, and there is a ▼ infant of: Document: {alinkColor: "", ...}. Window: {Infinity: Infinity, ...}

 >  console.log("Document: %o. Window: %o", document, window)
 <· ▼ Document: {alinkColor: "", ...}. Window: {Infinity: Infinity, ...}
     • ► {alinkColor: "", ...}
     • ► {Infinity: Infinity, ...}

We probably want to collapse that message by default.
Comment 19 Nikita Vasilyev 2015-02-01 21:59:57 PST
Created attachment 245858 [details]
WIP

Use `vertical-align: baseline` to keep monospace and non-monospace fonts nice and level on the same line.

https://cloudup.com/cT08nvJ25rz
Comment 20 Timothy Hatcher 2015-02-02 14:48:19 PST
Looks good.
Comment 21 WebKit Commit Bot 2015-02-11 19:01:37 PST
Attachment 245858 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/UserInterface/Views/LogContentView.js:459:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Views/LogContentView.js:540:  Line contains single-quote character.  [js/syntax] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 22 Nikita Vasilyev 2015-02-14 21:05:27 PST
Created attachment 246616 [details]
Patch

https://cldup.com/RR5k6HFZzz-3000x3000.png

This patch has grown big. I’d like to land it sooner than later and make a several consequent patched afterwards.
Comment 23 Joseph Pecoraro 2015-02-15 00:53:31 PST
(In reply to comment #22)
> Created attachment 246616 [details]
> Patch
> 
> https://cldup.com/RR5k6HFZzz-3000x3000.png

I like where this is going, but I see some issues.

Let me brain dump exactly what I think we should see in the Console, focusing on console messages.

Principles:

  - Be Concise. Strive for always having a single line output, offer expansion only if needed.
    - developers use console.loa lot. They want to quickly see values. Single line should be valued to get lots of data.

  - Messages and Values. Logging values is richer, so promote that use case.
    - A log might just contain a message, if so treat it like a message. console.error(msg) equivalent to a WebKit engine produced error message.
    - console.log has a format string to build a message. Lets use it for that purpose.
    - it is common to also log values, support rich output for values
    - currently logging a string value is not as useful as it could be. So make it valuable!

  - No Mixed Inline Object Expansion
    - Inline object expansion produces an unwieldy UI. One principle of the new console design is object expansions are always clean
    - This means that any console.log with an object needing expansion must handle object expansion gracefully

  - Log / Warn / Info / Error output is distinct from Console Evaluations
    - console.foo has a user provided message, so promote the message! (see above)
    - Comes from user source code, so include a Source Code Link / Backtrace
    - Distinct Icon + Color (Warn = Yellow, Error = Red, Info = Blue?, Log = ?)


All the cases I could think of, so we can discuss them by #:

    # Case 1: Log a Message (single line, not expandable, formatted as a message)
    > console.log("Entered Foo.bar()!")
    <· Entered Foo.bar()!

    # Case 2: Format String Message, no extra values (single line, not expandable, formatted as a message)
    > console.log("width: %d height: %d", w, h)
    <· width: 5 height: 10

    # Case 3: Log a single Primitive Value / Lossless Object (basically identical output to evaluating the object in the console yourself)
    > console.log(num)
    <· 1
    > console.log(arr)
    <· [1, 2, 3]
    > console.log(obj)
    <· {a: 1}

    # Case 4: Log multiple Primitive Values / Lossless Objects (basically identical output to evaluating the object in the console yourself, space separated)
    > console.log(num, arr, obj)
    <· 1 [1, 2, 3] {a: 1}

    # Case 5: Log a Message and any number of lossless values after it (single line, not expandable, message followed by values)
    > console.log("handled event", event.type, event.altKey, numClicks)
    <· handled event "click" false 12
    > console.log("handled %s event", event.type, event.altKey, numClicks)
    <· handled click event false 12

    ------ Now we get to anything that is lossy, and needs expansion to see everything ------

    # Case 6.1: Log a single Lossy Object - Collapsed (object preview)
    > console.log(window)
    <· ► Window {Infinity: Infinity, Array: Array, Document: Document...}

    # Case 6.2: Log a single Lossy Object - Expanded (object tree)
    > console.log(window)
    <· ▼ Window
         Infinity: Infinity
         Array: Array
         Document: Document
         ...

    # Case 7.1: Log multiple Lossy Objects - Collapsed (single line, brief object previews)
    > console.log(window, window.navigator)
    <· ► Window {Infinity: Infinity, ...} Navigator {appCodeName: "Mozilla", ...}

    # Case 7.2: Log multiple Lossy Objects - Expanded (multiple, separated non-brief object trees)
    > console.log(window, window.navigator)
    <· ▼ Logged Objects
       • ► Window {Infinity: Infinity, Array: Array, Document: Document...}
       • ► Navigator {appCodeName: "Mozilla", appName: "Netscape", language: "en-us"...}

    # Case 8.1: Message and Values with at least one Lossy Objects - Collapsed (single line, message, values, brief object previews)
    > console.log("%s event", event.type, performance.now(), window.navigator);
    <· ► click event 2659230.187729998 Navigator {appCodeName: "Mozilla", ...}

    # Case 8.2: Message and Values with at least one Lossy Objects - Expanded (multiple, separated values / object trees)
    > console.log("%s event", event.type, performance.now(), window.navigator);
    <· ▼ click event
       • 2659230.187729998
       • ► Navigator {appCodeName: "Mozilla", appName: "Netscape", language: "en-us"...}


    ------ Special considerations? ------

    # Special Case 1: Log String Value (Currently we cannot distinguish this from Case 1. Ideally it would be Case 3)
    > console.log(event.type)
    <· click

    # Special Case 2: Log String Literal (Currently we cannot distinguish this from Case 5. Maybe we should treat literals like messages?)
    > console.log("width", w, "height", h)
    <· width 5 "height" 10

    # Special Case 3.1: %o - Collapsed (Inline the object preview, brief if needed)
    > console.log("win: %o. nav: %o", window, navigator)
    <· ► win: Window {Infinity: Infinity, ...}. nav: Navigator {appCodeName: "Mozilla", ...}

    # Special Case 3.2: %o - Expanded (reduce object preview to class name, object trees separated)
    > console.log("win: %o. nav: %o", window, navigator)
    <· ► win: Window. nav: Navigator
       • ► Window {Infinity: Infinity, Array: Array, Document: Document...}
       • ► Navigator {appCodeName: "Mozilla", appName: "Netscape", language: "en-us"...}


Discussions to be had: (Would benefit from an implementation to play with)

  - Maybe we should skip past Case 7.1/8.1 and go immediately to 7.2/8.2
    - does this save time expanding, or does this cause multi-line output to too quickly fill the console

  - Maybe we should make the Object Previews in Case 7.1/8.1 clickable to immediately jump to a sub 7.2/8.2 faster
    - keep the collapse case, and make specific object expansion 1 click instead of 2
    - how would we show this in the UI? Give the entire preview a hover / clickable / token like state?

  - 7.2 string "Logged Objects" is totally a TBD string
    - would it be better to include the source code location? types of objects? just "Log"?


> This patch has grown big. I’d like to land it sooner than later and make a
> several consequent patched afterwards.

I'd much rather we start fresh anyways. I realize this started as tweaking styles, but I think it has become more than that. Trying to graft the new styles on top of the existing classes already looks like it is hard to write and for us to review. Also, this is an area of cruft that we have wanted to modernize for a long time anyways.

- ConsoleMessage/ConsoleMessageImpl blurs the lines between Model objects and View objects.
  - Currently we don't test console messages because they are considered "View" objects. That sucks! We want to test what we can.
  - I suggest we rename the existing objects LegacyConsoleMessage/LegacyConsoleMessageImpl and you can start to replace them with new objects:
    - Model/ConsoleMessage - abstraction for the Console.ConsoleMessage protocol type, created by ConsoleObserver, testable
    - Views/ConsoleMessageView - views for the message/previews/objects/sourceCodeLocation/backtrace that can added to the LogContentView

- This patch has a lot of FIXMEs / questions regarding the existing LogContentView / ConsoleMessage styles
  - if we start fresh, that shouldn't be the case. We will just add all new styles for the new non-legacy classes. Then we can wholesale remove the legacy version.
  - Console messages styles in LogContentView is confusing! I want to see something like ConsoleMessageView.css.
  - Object/Value styles being in LogContentView is confusing. They are used all over inspector not just console (e.g. debugger popovers)

- For the new console design we will want "ObjectPreview"s and "ObjectTree"s to eventually replace "ObjectPropertiesSection"s
  - The current object hierarchy tree needs a visual update:
    - It shows values poorly (functions)
    - edibility is cumbersome if it even works at all
    - expanding prototypes is cluttered, and often unnecessary
    - getters / setters aren't useful
  - ObjectPropertiesSection is used in many places in the inspector (console, sidebars, indexed databases). It can't be replaced immediately.
  - While creating a new ConsoleMessageView it would also makes sense to make the new ObjectTree/ObjectPreview in tandem so we have clean support for previews / trees in the console
    - it would take me another hour to type up the design we want for ObjectTree/ObjectPreview (currently only in email). I can do that when you are ready.


Sorry for the rant, I am just really looking forward to a rewrite here.
Comment 24 Timothy Hatcher 2015-02-15 01:23:24 PST
I agree with Joe's assessment. For Case 7.2, I think the string would just be "Window Navigator", like Special Case 3.2 without the format string.
Comment 25 Nikita Vasilyev 2015-02-15 19:51:43 PST
Joe,

Thanks for the comprehensive comment. It made me realize we were not exactly on the same page. It will take me some time to go through all the points.

Most importantly, my patch is rather a live prototype. It should let us make better design decisions.


>     # Case 7.1: Log multiple Lossy Objects - Collapsed (single line, brief
> object previews)
>     > console.log(window, window.navigator)
>     <· ► Window {Infinity: Infinity, ...} Navigator {appCodeName: "Mozilla",
> ...}

https://cloudup.com/cID0CgX951P
Notice how big are the previews. They don’t fit on a single line.

We could experiment with smaller previews to see if they are still being useful.


> > This patch has grown big. I’d like to land it sooner than later and make a
> > several consequent patched afterwards.
> 
> I'd much rather we start fresh anyways. I realize this started as tweaking
> styles, but I think it has become more than that. Trying to graft the new
> styles on top of the existing classes already looks like it is hard to write
> and for us to review. Also, this is an area of cruft that we have wanted to
> modernize for a long time anyways.
> 
> - ConsoleMessage/ConsoleMessageImpl blurs the lines between Model objects
> and View objects.
>   - Currently we don't test console messages because they are considered
> "View" objects. That sucks! We want to test what we can.
>   - I suggest we rename the existing objects
> LegacyConsoleMessage/LegacyConsoleMessageImpl and you can start to replace
> them with new objects:
>     - Model/ConsoleMessage - abstraction for the Console.ConsoleMessage
> protocol type, created by ConsoleObserver, testable
>     - Views/ConsoleMessageView - views for the
> message/previews/objects/sourceCodeLocation/backtrace that can added to the
> LogContentView
> 
> - This patch has a lot of FIXMEs / questions regarding the existing
> LogContentView / ConsoleMessage styles
>   - if we start fresh, that shouldn't be the case. We will just add all new
> styles for the new non-legacy classes. Then we can wholesale remove the
> legacy version.
>   - Console messages styles in LogContentView is confusing! I want to see
> something like ConsoleMessageView.css.
>   - Object/Value styles being in LogContentView is confusing. They are used
> all over inspector not just console (e.g. debugger popovers)
> 
> - For the new console design we will want "ObjectPreview"s and "ObjectTree"s
> to eventually replace "ObjectPropertiesSection"s
>   - The current object hierarchy tree needs a visual update:
>     - It shows values poorly (functions)
>     - edibility is cumbersome if it even works at all
>     - expanding prototypes is cluttered, and often unnecessary
>     - getters / setters aren't useful
>   - ObjectPropertiesSection is used in many places in the inspector
> (console, sidebars, indexed databases). It can't be replaced immediately.
>   - While creating a new ConsoleMessageView it would also makes sense to
> make the new ObjectTree/ObjectPreview in tandem so we have clean support for
> previews / trees in the console
>     - it would take me another hour to type up the design we want for
> ObjectTree/ObjectPreview (currently only in email). I can do that when you
> are ready.

Implementing the UI and these points are almost two different tasks altogether.

I’m still trying to understand how the existing code work, I can’t do a rewrite without knowing how it currently works. I was hoping to lend some changes incrementally. If you don’t want incremental changes, that’s fine. I will eventually do a rewrite, but it may take a couple of weeks.

I’ll get back to some of the points in your comment as I’m progressing with the patch.
Comment 26 Timothy Hatcher 2015-02-16 09:39:10 PST
Comment on attachment 246616 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ConsoleGroup.js:63
> +        //@FIXME
> +        element.messageElement = element;

Still needed I guess?

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:489
> -                if (i >= newRange[0] && i <= newRange[1] && !messageInRange.parentNode.classList.contains(WebInspector.LogContentView.SelectedStyleClassName)) {
> -                    messageInRange.parentNode.classList.add(WebInspector.LogContentView.SelectedStyleClassName);
> +                if (i >= newRange[0] && i <= newRange[1] && !messageInRange.classList.contains(WebInspector.LogContentView.SelectedStyleClassName)) {
> +                    messageInRange.classList.add(WebInspector.LogContentView.SelectedStyleClassName);
>                      this._selectedMessages.push(messageInRange);
> -                } else if (i < newRange[0] || i > newRange[1] && messageInRange.parentNode.classList.contains(WebInspector.LogContentView.SelectedStyleClassName)) {
> -                    messageInRange.parentNode.classList.remove(WebInspector.LogContentView.SelectedStyleClassName);
> +                } else if (i < newRange[0] || i > newRange[1] && messageInRange.classList.contains(WebInspector.LogContentView.SelectedStyleClassName)) {
> +                    messageInRange.classList.remove(WebInspector.LogContentView.SelectedStyleClassName);

Why was this needed? It seems like it would keep already selected elements out of the _selectedMessages array if you select some then select more in an inclusive range. Would _selectedMessages still include the old (still selected) messages?

> Source/WebInspectorUI/UserInterface/Views/Section.css:62
> +    line-height: 18px; /* @nvasilyev: Not sure why it is here. */

Likely a vertical-align hack for the disclosure triangles.
Comment 27 Nikita Vasilyev 2015-02-16 10:30:49 PST
Comment on attachment 246616 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ConsoleGroup.js:63
>> +        element.messageElement = element;
> 
> Still needed I guess?

Let me clean this up and remove messageElement since it’s redundant now.

>> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:489
>> +                    messageInRange.classList.remove(WebInspector.LogContentView.SelectedStyleClassName);
> 
> Why was this needed? It seems like it would keep already selected elements out of the _selectedMessages array if you select some then select more in an inclusive range. Would _selectedMessages still include the old (still selected) messages?

I only removed parentNode (e.g s/messageInRange.parentNode/messageInRange/g), everything else is unchanged.
Comment 28 Nikita Vasilyev 2015-02-16 10:33:08 PST
Comment on attachment 246616 [details]
Patch

I’ll clean up ConsoleGroup.js first a little bit.
Comment 29 Nikita Vasilyev 2015-02-16 18:37:50 PST
Created attachment 246714 [details]
Patch
Comment 30 WebKit Commit Bot 2015-02-16 20:20:32 PST
Comment on attachment 246714 [details]
Patch

Clearing flags on attachment: 246714

Committed r180205: <http://trac.webkit.org/changeset/180205>
Comment 31 WebKit Commit Bot 2015-02-16 20:20:38 PST
All reviewed patches have been landed.  Closing bug.