760876 - Can't drag-select multiple messages in the Web Console

Status: VERIFIED FIXED

(DevTools :: Console, defect, P1)

Description

[Jesse Ruderman]

1. Open the Web Console
2. Load a page, so the Net console shows some messages.
3. Try to select all the messages by dragging from the top to the bottom.

Result: Only the mousedown-ed message becomes selected.

This leads users to taking screenshots instead of copying+pasting (if they don't discover that select-all works).

Tested using Firefox Nightly 15.0a1 (2012-06-02)

Comment 1

[Mihai Sucan [:msucan]]

This seems to be a duplicate of bug 706755. We need to rewrite parts of the Web Console output to achieve the desired user experience.

Comment 2

[Mihai Sucan [:msucan]]

Please reopen if you believe this is not a duplicate.

Comment 3

[Jesse Ruderman]

Selecting items and selecting code sound like different things to me.  I'd rather have this as a dependency, but I won't complain if one patch fixes both problems :)

Comment 4

[Mihai Sucan [:msucan]]

Attachment: part 1 - switch from xul to xhtml v0.1

Changes in this patch:

- added API to ConsoleOutput for determining the message DOM element based on any descendant.
- added API to ConsoleOutput to retrieve an array of selected messages (even if the selection starts and ends in the middle of the same or different messages). This method is needed for compatibility with existing web console APIs, and it will be helpful for tests as well.
- added API to ConsoleOutput to select all messages.
- changed all occurrences of createElement*() to create appropriate XHTML elements - eg. using anchors for links.
- simplified DOM structure for some message types:
-- removed .webconsole-msg-repeat from messages that cannot have repeats.
-- simplified the icon and border section for each message.
-- simplified the network messages which had arcane wrapping and classes for every item displayed.
-- similar simplification for console.dir() output.
- added tooltips for navigation marker URL, and for message repeat element (bug 675487).
- switched away from querySelector*() to getElementsBy*() where possible to improve performance.
- updated web console styling to work with the new XHTML elements. This means I had to use css3 flexbox (yay, no longer buggy old XUL flexbox).
-- unfortunately this means no more crop=center (which is implemented for xul:labels in C++). We only have CSS text-overflow: ellipsis which crops text at the end.
-- this aggravated the way the message location is displayed when it has a hash, seebug 724224. I included a fix for that, and for data URIs.
-- these changes also causes links to not be focusable by Tab in output. Will submit a fix in a separate patch.

More changes are coming in separate patches.

Should we ask someone for css review? joe, paul or dao?

Comment 5

[Mihai Sucan [:msucan]]

Attachment: part 2 - fix links

Changes in this patch:

- selection can start inside all links (draggable=false).
- add href=url to all anchors. All links become focusable by Tab and this will allow us to implement a generic event handler to open links from the console, in new tabs. This is a (partial?) fix for bug 588010.
- changed the target for event listeners, so you can only click actual links - fix for bug 773291.

Comment 6

[Mihai Sucan [:msucan]]

Attachment: part 3 - cleanup CSS classes v0.1

Changes in this patch:

- removed .hud- and .webconsole- prefixes, cleaned-up classes.
- removed class names for categories, severities and filters. It was a lot of confusion. Switched to attributes.

Will start work on fixing all of the tests. When ready, I will submit the 4th patch. Before that, I will address review comments for bug 877262.

With all these patches, play with the console, check copy/paste and so on.

I was initially worried that losing the formatted clipboard text for each message might badly degrade the quality/usability of the text you can copy from output. Actually, it seems nice.

Comment 7

[Mihai Sucan [:msucan]]

Patch queue: bug 877262, bug 793996 then these patches.

Comment 8

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

Comment on attachment 774239 [details] [diff] [review]
part 1 - switch from xul to xhtml v0.1

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

::: browser/devtools/webconsole/console-output.js
@@ +365,2 @@
>  
> +    this.$render().element.appendChild(urlnode);

Messages.BaseMessage.prototype.render().element.appendChild(urlNode);

::: browser/devtools/webconsole/webconsole.js
@@ +918,5 @@
>     */
>    mergeFilteredMessageNode:
>    function WCF_mergeFilteredMessageNode(aOriginal, aFiltered)
>    {
> +    let repeatNode = aOriginal.getElementsByClassName("webconsole-msg-repeat")[0];

I think we can use aOriginal.querySelector("webconsole-msg-repeat") now. Might have to QI it.

@@ +1151,5 @@
>  
>      if (objectActors.size > 0) {
>        node._objectActors = objectActors;
>  
> +      let repeatNode = node.getElementsByClassName("webconsole-msg-repeat")[0];

same here?

@@ +2246,3 @@
>      timestampNode.classList.add("webconsole-timestamp");
> +    // Apply the current group by indenting appropriately.
> +    timestampNode.style.marginRight = this.groupDepth * GROUP_INDENT + "px";

indenting by applying marginRight or left?

@@ +2310,5 @@
>    function WCF__makeConsoleLogMessageBody(aMessage, aContainer, aBody)
>    {
>      Object.defineProperty(aMessage, "_panelOpen", {
>        get: function() {
> +        let nodes = aContainer.getElementsByTagName("a");

does qSA just not work on an XHTML document?

::: browser/devtools/webconsole/webconsole.xul
@@ +75,5 @@
>        <menuitem id="cMenu_selectAll"/>
>      </menupopup>
>    </popupset>
>  
> +  <tooltip id="aHTMLTooltip" page="true"/>

that's a funny lookin' id.

::: browser/themes/shared/devtools/webconsole.inc.css
@@ +22,2 @@
>    font-family: monospace;
> +  font-size: 0.9em;

shouldn't this be system size? (are we supposed to use ems for units here?)

Comment 9

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

Comment on attachment 774248 [details] [diff] [review]
part 2 - fix links

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

::: browser/devtools/webconsole/console-output.js
@@ +365,5 @@
> +    urlnode.href = this._url;
> +    urlnode.draggable = false;
> +
> +    this.output.owner._addMessageLinkCallback(urlnode, () => {
> +      this.output.owner.owner.openLink(this._url);

This is a bit of a nasty lookup. Should comment this so we know what output.owner.owner is (hudservice?).

::: browser/devtools/webconsole/webconsole.js
@@ +1362,5 @@
>      let statusNode = this.document.createElementNS(XHTML_NS, "a");
>      statusNode.classList.add("webconsole-msg-status");
>      body.appendChild(statusNode);
>  
> +    let onClick = () => {

that's so much nicer.

@@ +2564,5 @@
> +
> +      // If this event started with a mousedown event and it ends at a different
> +      // location, we consider this text selection.
> +      if (mousedown && this._startX != aEvent.clientX &&
> +          this._startY != aEvent.clientY) {

nice. This finally fixes the "clicking on a line causes the network panel to open" bug. (e.g., bug 773291).

Comment 10

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

Comment on attachment 774257 [details] [diff] [review]
part 3 - cleanup CSS classes v0.1

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

not sure about the css changes, I need to look at it more closely. Additional ancestor selectors are a little disconcerting, but probably not a huge deal.

I am attaching a screenshot showing before and after.

::: browser/devtools/webconsole/webconsole.js
@@ +478,5 @@
>  
>      let doc = this.document;
>  
>      this.filterBox = doc.querySelector(".hud-filter-box");
> +    this.outputNode = doc.querySelector("#output-container");

can use getElementById here, if you like.

Comment 11

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

Attachment: before.png

Comment 12

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

Attachment: after.png

Comment 13

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

not sure if this is a hidpi thing or not. Will test on a non-retina mac later.

Comment 14

[Mihai Sucan [:msucan]]

Thanks for the reviews!

(In reply to Rob Campbell [:rc] (:robcee) from comment #8)
> Comment on attachment 774239 [details] [diff] [review]
> part 1 - switch from xul to xhtml v0.1
> 
> Review of attachment 774239 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/webconsole/console-output.js
> @@ +365,2 @@
> >  
> > +    this.$render().element.appendChild(urlnode);
> 
> Messages.BaseMessage.prototype.render().element.appendChild(urlNode);

Will fix.


> ::: browser/devtools/webconsole/webconsole.js
> @@ +918,5 @@
> >     */
> >    mergeFilteredMessageNode:
> >    function WCF_mergeFilteredMessageNode(aOriginal, aFiltered)
> >    {
> > +    let repeatNode = aOriginal.getElementsByClassName("webconsole-msg-repeat")[0];
> 
> I think we can use aOriginal.querySelector("webconsole-msg-repeat") now.
> Might have to QI it.

I'm switching away from querySelector()/querySelectorAll() to more specific methods of element finding for performance reasons. In performance-related bugs that bz has reported we've been suggested to avoid qS/qSA() in output code to improve speed, along with keeping layout and reflows to a minimum.


> @@ +2246,3 @@
> >      timestampNode.classList.add("webconsole-timestamp");
> > +    // Apply the current group by indenting appropriately.
> > +    timestampNode.style.marginRight = this.groupDepth * GROUP_INDENT + "px";
> 
> indenting by applying marginRight or left?

In this patch we are changing the styling quit much, and now we need marginRight.


> @@ +2310,5 @@
> >    function WCF__makeConsoleLogMessageBody(aMessage, aContainer, aBody)
> >    {
> >      Object.defineProperty(aMessage, "_panelOpen", {
> >        get: function() {
> > +        let nodes = aContainer.getElementsByTagName("a");
> 
> does qSA just not work on an XHTML document?

It does. See above why I am not using it.


> ::: browser/devtools/webconsole/webconsole.xul
> @@ +75,5 @@
> >        <menuitem id="cMenu_selectAll"/>
> >      </menupopup>
> >    </popupset>
> >  
> > +  <tooltip id="aHTMLTooltip" page="true"/>
> 
> that's a funny lookin' id.

Picked the ID based on other documents that use the same element for the same purpose. If you want a different ID please let me know.


> ::: browser/themes/shared/devtools/webconsole.inc.css
> @@ +22,2 @@
> >    font-family: monospace;
> > +  font-size: 0.9em;
> 
> shouldn't this be system size? (are we supposed to use ems for units here?)

1em is equal to the current font-size of the XUL document (which is the system based font-size). If I change my system font size, the web console output font size also changes.

We can use percentages or ems or any other relative unit. ems worked for me nicely. Please let me know if you want any specific changes here.

Comment 15

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #9)
> Comment on attachment 774248 [details] [diff] [review]
> part 2 - fix links
> 
> Review of attachment 774248 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/webconsole/console-output.js
> @@ +365,5 @@
> > +    urlnode.href = this._url;
> > +    urlnode.draggable = false;
> > +
> > +    this.output.owner._addMessageLinkCallback(urlnode, () => {
> > +      this.output.owner.owner.openLink(this._url);
> 
> This is a bit of a nasty lookup. Should comment this so we know what
> output.owner.owner is (hudservice?).

This is, indeed, a bit of a nasty lookup. Until we progress further I need to keep the code as-is - otherwise I would add quite more churn in these patches.

I'll add a comment.


> ::: browser/devtools/webconsole/webconsole.js
> @@ +1362,5 @@
> >      let statusNode = this.document.createElementNS(XHTML_NS, "a");
> >      statusNode.classList.add("webconsole-msg-status");
> >      body.appendChild(statusNode);
> >  
> > +    let onClick = () => {
> 
> that's so much nicer.

Yes!

> @@ +2564,5 @@
> > +
> > +      // If this event started with a mousedown event and it ends at a different
> > +      // location, we consider this text selection.
> > +      if (mousedown && this._startX != aEvent.clientX &&
> > +          this._startY != aEvent.clientY) {
> 
> nice. This finally fixes the "clicking on a line causes the network panel to
> open" bug. (e.g., bug 773291).

Indeed, finally. It feels good to fix long-standing issues.

Comment 16

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #10)
> Comment on attachment 774257 [details] [diff] [review]
> part 3 - cleanup CSS classes v0.1
> 
> Review of attachment 774257 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> not sure about the css changes, I need to look at it more closely.
> Additional ancestor selectors are a little disconcerting, but probably not a
> huge deal.

I am not sure how bad the use of ancestor selectors affects output performance. I expect it's not much of an impact (is there a way to measure this?) because these styles only apply to the webconsole iframe and we limit the number of messages visible at once in output. The reason why I use them is simple: if you have a message with a .timestamp element inside .body I do not want to endup styling the .body contents by mistake - I only want to style the .message > .timestamp element. I also wanted to avoid arcane/superfluous class names like .message-timestamp or .webconsole-msg-foo.

I can update the patch to avoid ancestor selectors. Should we ask Dão for review as well?


> I am attaching a screenshot showing before and after.
> 
> ::: browser/devtools/webconsole/webconsole.js
> @@ +478,5 @@
> >  
> >      let doc = this.document;
> >  
> >      this.filterBox = doc.querySelector(".hud-filter-box");
> > +    this.outputNode = doc.querySelector("#output-container");
> 
> can use getElementById here, if you like.

Will do.

Comment 17

[Mihai Sucan [:msucan]]

Attachment: part 1 - switch from xul to xhtml v0.2

Rebased the patch, addressed review comments and some fixes based on the work I am doing for the "part 4 - test fixes" patch.

Comment 18

[Mihai Sucan [:msucan]]

Attachment: part 2 - fix links v0.2

Rebased the patch.

Comment 19

[Mihai Sucan [:msucan]]

Attachment: part 3 - cleanup CSS classes v0.2

Rebased the patch and addressed review comments. If you want I can remove the use of ancestor selectors - see comment 16.

I still need to test these patches on MacOS. I will do that once I also have the "part 4 - test fixes" patch ready. I will also look into the baseline alignment issues you pointed out.

Thank you!

Comment 20

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

Comment on attachment 786897 [details] [diff] [review]
part 1 - switch from xul to xhtml v0.2

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

I'd like someone else to look over the style code in this, but it looks decent.

::: browser/devtools/webconsole/webconsole.js
@@ +919,5 @@
>     */
>    mergeFilteredMessageNode:
>    function WCF_mergeFilteredMessageNode(aOriginal, aFiltered)
>    {
> +    let repeatNode = aOriginal.getElementsByClassName("webconsole-msg-repeat")[0];

now that we're in an xhtml document, can we use querySelector for this?

@@ +1069,5 @@
>  
>          body = l10n.getFormatStr("stacktrace.outputMessage",
>                                   [filename, functionName, sourceLine]);
>  
> +        clipboardText = body + "\n";

why does this need a newline in it?

@@ +1152,5 @@
>  
>      if (objectActors.size > 0) {
>        node._objectActors = objectActors;
>  
> +      let repeatNode = node.getElementsByClassName("webconsole-msg-repeat")[0];

same, can we use querySelector?

@@ +1920,5 @@
>      // Prune messages if needed. We do not do this for every flush call to
>      // improve performance.
>      let removedNodes = 0;
>      if (shouldPrune || !this._outputQueue.length) {
> +      oldScrollHeight = outputNode.scrollHeight;

bleh. I had to look up scrollHeight again. Not part of any standard.

@@ +2363,5 @@
>    function WCF__makeConsoleLogMessageBody(aMessage, aContainer, aBody)
>    {
>      Object.defineProperty(aMessage, "_panelOpen", {
>        get: function() {
> +        let nodes = aContainer.getElementsByTagName("a");

could still use qSA here.

::: browser/devtools/webconsole/webconsole.xul
@@ +166,5 @@
>        </toolbar>
>  
> +      <hbox id="output-wrapper" flex="1" context="output-contextmenu" tooltip="aHTMLTooltip">
> +        <div xmlns="http://www.w3.org/1999/xhtml"
> +             class="hud-output-node" tabindex="1"/>

you're namespacing this. This is the output node. It doesn't contain any additional iframe does it all of the message nodes end up here?

we could get rid of the createElementNS calls all over in webconsole.js if we cared about that by adding an appropriately namespaced document in an iframe. Not sure I care about that though.

Comment 21

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

ugh. sorry about the querySelector comments. I forgot about your comment #14. Disregard those.

Comment 22

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #20)
> ::: browser/devtools/webconsole/webconsole.xul
> @@ +166,5 @@
> >        </toolbar>
> >  
> > +      <hbox id="output-wrapper" flex="1" context="output-contextmenu" tooltip="aHTMLTooltip">
> > +        <div xmlns="http://www.w3.org/1999/xhtml"
> > +             class="hud-output-node" tabindex="1"/>
> 
> you're namespacing this. This is the output node. It doesn't contain any
> additional iframe does it all of the message nodes end up here?
> 
> we could get rid of the createElementNS calls all over in webconsole.js if
> we cared about that by adding an appropriately namespaced document in an
> iframe. Not sure I care about that though.

I doubt we care. Nesting output into an iframe would be more expensive in terms of resources, iianm - it would be a new document. Do you think we should do this?

All messages end up in the HTML output node. Thanks for the review!

Comment 23

[Mihai Sucan [:msucan]]

Attachment: part 1 - switch from xul to xhtml v0.3

Rob: This patch is a rebase and a minor fix for the output icons alignment issue (tested on macosx 10.7). Please retest to check if the icons look right for you. Thanks!

Paul: Rob suggests I ask you to review the CSS changes. Can you please do this?

I also have a problem that I need help with from you. Please apply all the four patches in this bug and test the console output. Evaluate |new Array(200000).join("a")|, then expand the long string output result. Notice that the whole browser freezes. If you qpop all these patches, the same use-case works well - instant rendering. Do you know why and how we can avoid this problem? Is CSS 3 flexbox soooo slow?

Thank you!

Comment 24

[Mihai Sucan [:msucan]]

Attachment: part 2 - fix links v0.3

rebased the patch

Comment 25

[Mihai Sucan [:msucan]]

Attachment: part 3 - cleanup CSS classes v0.3

rebased the patch

Comment 26

[Mihai Sucan [:msucan]]

Attachment: part 4 - fix the tests v0.1

Fun with tests! :)

First try push: https://tbpl.mozilla.org/?tree=Try&rev=3bd79d68815b

Do note that I removed some tests that seem no longer relevant. Please feel free to suggest otherwise.

Comment 27

[Paul Rouget [:paul]]

Comment on attachment 793051 [details] [diff] [review]
part 1 - switch from xul to xhtml v0.3

> +  width: calc(100% - 6px - 8px);

If you use `box-sizing:border-box;`, do you still need this calc()?

> + /* Make sure the message body is very small initially

That's weird. What if you use `flex-grow:1`? Do you still need it?

---

No need to address this now:

> .webconsole-msg-url

Can you make it overflow on the left? Maybe with `align-self:end` (never tried), or forcing a rtl direction?

Comment 28

[Brian Grinstead [:bgrins]]

(In reply to Paul Rouget [:paul] from comment #27)
> Comment on attachment 793051 [details] [diff] [review]
> part 1 - switch from xul to xhtml v0.3
> 
> > +  width: calc(100% - 6px - 8px);
> 
> If you use `box-sizing:border-box;`, do you still need this calc()?
> 

border-box will not include margins, but it will include paddings / borders.  So you could use this code to avoid the repeated constants and get rid of the calc() call:

.hud-msg-node {
  display: flex;
  flex: 0 0 auto;
  -moz-padding-start: 6px;
  -moz-padding-end: 8px;
  width: 100%;
  box-size: border-box;
}

Comment 29

[Brian Grinstead [:bgrins]]

> .hud-msg-node {
>   display: flex;
>   flex: 0 0 auto;
>   -moz-padding-start: 6px;
>   -moz-padding-end: 8px;
>   width: 100%;
>   box-size: border-box;
> }

Had a typo there on box-size.  Should be

  -moz-box-sizing: border-box;

Comment 30

[Paul Rouget [:paul]]

(In reply to Paul Rouget [:paul] from comment #27)
> > + /* Make sure the message body is very small initially
> 
> That's weird. What if you use `flex-grow:1`? Do you still need it?

Maybe a `flex-shrink:0` will avoid your box to be collapsed.

Comment 31

[Brian Grinstead [:bgrins]]

Attachment: console-760876.patch

A proposed performance fix for allowing expansion of long messages like `new Array(200000).join("a")`.  Does this fix the performance issues for you?

Comment 32

[Mihai Sucan [:msucan]]

(In reply to Paul Rouget [:paul] from comment #27)
> Comment on attachment 793051 [details] [diff] [review]
> part 1 - switch from xul to xhtml v0.3
> 
> > +  width: calc(100% - 6px - 8px);
> 
> If you use `box-sizing:border-box;`, do you still need this calc()?

Yes. border-box only includes padding and borders.


> > + /* Make sure the message body is very small initially
> 
> That's weird. What if you use `flex-grow:1`? Do you still need it?

This is changing in the upcoming patch. flex-grow:1 did not work for me.


> 
> No need to address this now:
> 
> > .webconsole-msg-url
> 
> Can you make it overflow on the left? Maybe with `align-self:end` (never
> tried), or forcing a rtl direction?

I tried this, but I get texts like http://example.com/ rendered as /http://example.com and I haven't found a way to prevent this kind of problems with punctuation and rtl texts. Maybe I am missing something.

Thanks for your review!


(In reply to Brian Grinstead [:bgrins] from comment #31)
> Created attachment 794037 [details] [diff] [review]
> console-760876.patch
> 
> A proposed performance fix for allowing expansion of long messages like `new
> Array(200000).join("a")`.  Does this fix the performance issues for you?

Thank you! This is working, but I am worried we will see some side effects of this workaround. Until then, we'll use this approach.

Comment 33

[Mihai Sucan [:msucan]]

Attachment: part 1 - switch from xul to xhtml v0.4

Rebased patch and included a change suggested by Paul: network requests no longer show their query ?parameters. This makes the output cleaner when network logs show up.

I'm also including Brian's render performance workaround.

Comment 34

[Mihai Sucan [:msucan]]

Attachment: part 2 - fix links v0.4

Rebased patch.

Comment 35

[Mihai Sucan [:msucan]]

Attachment: part 3 - cleanup CSS classes v0.4

Rebased the patch.

Comment 36

[Mihai Sucan [:msucan]]

Attachment: part 4 - fix the tests v0.2

More fixes for tests based on try server runs.

Latest results: https://tbpl.mozilla.org/?tree=Try&rev=820531ff781f

Green results with opt builds while all debug builds are busted because of the tbpl error: 'Output exceeded 52428800 bytes, remaining output has been truncated'

It seems that the new code for the web console output generates more warnings in debug builds -- too many for tbpl. I'm not sure how I can fix this issue.

Ryan, can you please look into the tbpl run and let me know of anything I could do here? Thank you!

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Not sure what you expect me to tell you besides "fix the warnings" ?

Comment 38

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #37)
> Not sure what you expect me to tell you besides "fix the warnings" ?

That's not a really helpful reply. If you have pointers on how to fix this type of error, it would be welcome. I don't think we've seen it before and it appears build infrastructure related.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

The issue is that your test is spewing a bunch of extra warnings in what appears to be layout code? Try asking a layout developer.

Comment 40

[Mihai Sucan [:msucan]]

Thank you Ryan. I have found a way to avoid the spew of warnings in debug builds.

Comment 41

[Mihai Sucan [:msucan]]

Attachment: part 3 - cleanup CSS classes v0.5

Removed the display:table wrapper - this is what caused the tons of warnings for every new message being rendered in the web console output. To avoid the performance issues I am no longer forcing word-wrap: if the user tries to output veeery long words he will see a vertical scrollbar which is really the same behavior as without these patches. Word wrap is slow...

Comment 42

[Mihai Sucan [:msucan]]

Attachment: part 4 - fix the tests v0.3

Rebased the patch.

Two green try pushes:
https://tbpl.mozilla.org/?tree=Try&rev=4255177641f7
https://tbpl.mozilla.org/?tree=Try&rev=1f991c8e3bd4

Comment 43

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

Attachment: Screen Shot 2013-09-12 at 1.57.14 PM.png

showing vertical mis-alignment on timestamps and text.

Comment 44

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

Comment on attachment 800236 [details] [diff] [review]
part 1 - switch from xul to xhtml v0.4

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

::: browser/devtools/webconsole/console-output.js
@@ +57,5 @@
>    /**
> +   * The output container.
> +   * @type DOMElement
> +   */
> +  get element() this.owner.outputNode,

please avoid getter expressions. Use your { }s!

Comment 45

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

Comment on attachment 803611 [details] [diff] [review]
part 3 - cleanup CSS classes v0.5

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

::: browser/devtools/webconsole/webconsole.js
@@ +847,5 @@
> +    // (filter="error", filter="cssparser", etc.) and add or remove the
> +    // "filtered-by-type" class, which turns on or off the display.
> +
> +    let xpath = ".//*[contains(@class, 'message') and " +
> +      "@filter='" + aPrefKey + "']";

thank goodness our xpath selectors survived. :)

Comment 46

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

Comment on attachment 803612 [details] [diff] [review]
part 4 - fix the tests v0.3

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

oh god.

Comment 47

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #45)
> Comment on attachment 803611 [details] [diff] [review]
> part 3 - cleanup CSS classes v0.5
> 
> Review of attachment 803611 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/webconsole/webconsole.js
> @@ +847,5 @@
> > +    // (filter="error", filter="cssparser", etc.) and add or remove the
> > +    // "filtered-by-type" class, which turns on or off the display.
> > +
> > +    let xpath = ".//*[contains(@class, 'message') and " +
> > +      "@filter='" + aPrefKey + "']";
> 
> thank goodness our xpath selectors survived. :)

hehe, not for long, I hope! :)

thanks for the reviews!

Comment 48

[Mihai Sucan [:msucan]]

Attachment: part 3 - cleanup CSS classes v0.6

Included a fix for the timestamps alignment.

Comment 49

[Mihai Sucan [:msucan]]

Landed:
https://hg.mozilla.org/integration/fx-team/rev/9d115a405371
https://hg.mozilla.org/integration/fx-team/rev/c68a3f506dbd
https://hg.mozilla.org/integration/fx-team/rev/a4e288cfa8d3
https://hg.mozilla.org/integration/fx-team/rev/e7c825ddd540

Thank you Rob, Paul and Brian!

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9d115a405371
https://hg.mozilla.org/mozilla-central/rev/c68a3f506dbd
https://hg.mozilla.org/mozilla-central/rev/a4e288cfa8d3
https://hg.mozilla.org/mozilla-central/rev/e7c825ddd540

Comment 52

[Mihai Morar, (:MihaiMorar)]

Verified as Fixed on Latest Nightly 26 using Windows 7, Mac OS 10.7.5 and Ubuntu 13.04
BuildID: 20130915030208