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

VERIFIED FIXED in Firefox 26

Status

defect
P1
normal
VERIFIED FIXED
7 years ago
Last year

People

(Reporter: jruderman, Assigned: msucan)

Tracking

Trunk
Firefox 26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 13 obsolete attachments)

79.42 KB, image/png
Details
113.85 KB, image/png
Details
2.23 KB, patch
Details | Diff | Splinter Review
66.45 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review
18.20 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review
186.91 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review
130.58 KB, image/png
Details
40.24 KB, patch
Details | Diff | Splinter Review
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)
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.
Please reopen if you believe this is not a duplicate.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 706755
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 :)
Status: RESOLVED → REOPENED
Depends on: 706755
Resolution: DUPLICATE → ---
Depends on: console-output
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86_64 → All
Assignee: nobody → mihai.sucan
Status: REOPENED → ASSIGNED
Blocks: 724224
Blocks: 675487
Blocks: 628019
Blocks: 773291
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?
Attachment #774239 - Flags: review?(rcampbell)
Attachment #774239 - Attachment description: part 1 - switch from xul to v0.1 → part 1 - switch from xul to xhtml v0.1
Posted patch part 2 - fix links (obsolete) — Splinter Review
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.
Attachment #774248 - Flags: review?(rcampbell)
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.
Attachment #774257 - Flags: review?(rcampbell)
Patch queue: bug 877262, bug 793996 then these patches.
Depends on: 793996
Blocks: 722267
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?)
Attachment #774239 - Flags: review?(rcampbell)
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).
Attachment #774248 - Flags: review?(rcampbell) → review+
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.
Attachment #774257 - Flags: review?(rcampbell)
not sure if this is a hidpi thing or not. Will test on a non-retina mac later.
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.
(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.
(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.
Rebased the patch, addressed review comments and some fixes based on the work I am doing for the "part 4 - test fixes" patch.
Attachment #774239 - Attachment is obsolete: true
Attachment #786897 - Flags: review?(rcampbell)
Posted patch part 2 - fix links v0.2 (obsolete) — Splinter Review
Rebased the patch.
Attachment #774248 - Attachment is obsolete: true
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!
Attachment #774257 - Attachment is obsolete: true
Attachment #786899 - Flags: review?(rcampbell)
Blocks: 886836
Blocks: 874591
Blocks: 903676
Blocks: 872318
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.
Attachment #786897 - Flags: review?(rcampbell) → review+
ugh. sorry about the querySelector comments. I forgot about your comment #14. Disregard those.
(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!
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!
Attachment #786897 - Attachment is obsolete: true
Attachment #793051 - Flags: review?(paul)
Posted patch part 2 - fix links v0.3 (obsolete) — Splinter Review
rebased the patch
Attachment #786898 - Attachment is obsolete: true
rebased the patch
Attachment #786899 - Attachment is obsolete: true
Attachment #786899 - Flags: review?(rcampbell)
Attachment #793058 - Flags: review?(rcampbell)
Posted patch part 4 - fix the tests v0.1 (obsolete) — Splinter Review
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.
Attachment #793064 - Flags: review?(rcampbell)
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?
Attachment #793051 - Flags: review?(paul) → review+
(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;
}
> .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;
(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.
A proposed performance fix for allowing expansion of long messages like `new Array(200000).join("a")`.  Does this fix the performance issues for you?
Flags: needinfo?(mihai.sucan)
(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.
Flags: needinfo?(mihai.sucan)
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.
Attachment #793051 - Attachment is obsolete: true
Rebased patch.
Attachment #793053 - Attachment is obsolete: true
Rebased the patch.
Attachment #793058 - Attachment is obsolete: true
Attachment #793058 - Flags: review?(rcampbell)
Attachment #800239 - Flags: review?(rcampbell)
Posted patch part 4 - fix the tests v0.2 (obsolete) — Splinter Review
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!
Attachment #793064 - Attachment is obsolete: true
Attachment #793064 - Flags: review?(rcampbell)
Attachment #800254 - Flags: review?(rcampbell)
Flags: needinfo?(ryanvm)
Not sure what you expect me to tell you besides "fix the warnings" ?
Flags: needinfo?(ryanvm)
(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.
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.
Blocks: 762608
Thank you Ryan. I have found a way to avoid the spew of warnings in debug builds.
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...
Attachment #800239 - Attachment is obsolete: true
Attachment #800239 - Flags: review?(rcampbell)
Attachment #803611 - Flags: review?(rcampbell)
Rebased the patch.

Two green try pushes:
https://tbpl.mozilla.org/?tree=Try&rev=4255177641f7
https://tbpl.mozilla.org/?tree=Try&rev=1f991c8e3bd4
Attachment #800254 - Attachment is obsolete: true
Attachment #800254 - Flags: review?(rcampbell)
Attachment #803612 - Flags: review?(rcampbell)
showing vertical mis-alignment on timestamps and text.
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!
Attachment #800236 - Flags: review+
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. :)
Attachment #803611 - Flags: review?(rcampbell) → review+
Comment on attachment 803612 [details] [diff] [review]
part 4 - fix the tests v0.3

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

oh god.
Attachment #803612 - Flags: review?(rcampbell) → review+
(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!
Included a fix for the timestamps alignment.
Attachment #803611 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
Depends on: 916329
Duplicate of this bug: 872318
No longer blocks: 903676
Verified as Fixed on Latest Nightly 26 using Windows 7, Mac OS 10.7.5 and Ubuntu 13.04
BuildID: 20130915030208
Status: RESOLVED → VERIFIED
QA Contact: mihai.morar
Blocks: 1208734
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.