Bug 140580 – Web Inspector: Tweak the styles on the Console

• Component: Web Inspector
• Status: RESOLVED FIXED

[Timothy Hatcher]

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/

[Radar WebKit Bug Importer]

<rdar://problem/19507400>

[Timothy Hatcher]

See bug 100302 for Nikita's old attempt at some of this, that was rejected by Chrome dev.

Lets prevail!

[Nikita Vasilyev]

Created attachment 245468 [details]
WIP

[Nikita Vasilyev]

Created attachment 245469 [details]
[Image] WIP

Screenshot of the WIP patch applied.

[Nikita Vasilyev]

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;

[Nikita Vasilyev]

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?

[Nikita Vasilyev]

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.

[Joseph Pecoraro]

(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.

[Nikita Vasilyev]

(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.

[Timothy Hatcher]

(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.

[Timothy Hatcher]

> 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.

[Timothy Hatcher]

(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?

[Timothy Hatcher]

But maybe our popovers just needs some work. They are a little flaky.

[Timothy Hatcher]

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.

[Nikita Vasilyev]

(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.

[Nikita Vasilyev]

(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.

?

[Nikita Vasilyev]

(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.

[Timothy Hatcher]

(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.

[Nikita Vasilyev]

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

[Timothy Hatcher]

Looks good.

[WebKit Commit Bot]

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.

[Nikita Vasilyev]

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.

[Joseph Pecoraro]

(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.

[Timothy Hatcher]

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.

[Nikita Vasilyev]

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.

[Timothy Hatcher]

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.

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

Comment on attachment 246616 [details]
Patch

I’ll clean up ConsoleGroup.js first a little bit.

[Nikita Vasilyev]

Created attachment 246714 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 246714 [details]
Patch

Clearing flags on attachment: 246714

Committed r180205: <http://trac.webkit.org/changeset/180205>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.