Closed Bug 920321 Opened 6 years ago Closed 6 years ago

line number is ellipsized in console

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 28

People

(Reporter: vlad, Assigned: thomas)

References

Details

(Whiteboard: [good first bug][lang=js,html,css][mentor=msucan][good first verify])

Attachments

(2 files, 2 obsolete files)

See http://imgur.com/UPrrs7n -- the line number is perhaps the most useful part, and should basically never be ellipsized.
Duplicate of this bug: 923021
OS: Windows 8 → All
Hardware: x86 → All
Priority: -- → P3
Whiteboard: [good first bug][lang=js,html,css][mentor=msucan]
Hi Mihai

I would like to look at this, but does this issue exist in 24?
See the screenshot
Hello Thomas! Thanks for your interest to take this bug.

This bug was introduced by bug 760876, in Firefox 26, when we switched the console output from XUL to XHTML+more CSS than before. We had <xul:label crop=middle> which added an ellipsis in the middle of the text - unfortunately, there's no support for changing the ellipsis location in XHTML+CSS (that was a xul-only capability).

To fix this bug you need to look into browser/devtools/webconsole/webconsole.js - search for WCF_createLocationNode(). Also look into browser/themes/shared/devtools/webconsole.inc.css. I believe you should split the anchor that holds the fileName:lineNumber string into two separate elements. This should allow you to add the ellipsis only to the file name.

Please let me know if you have any further questions. Thank you!


(IIANM, this wouldn't be the first patch you would work on for us? I can spare you the boring/usual introduction. :) )
Attached patch 920321-initial.patch (obsolete) — Splinter Review
Hi Mihai

Here is the first try on this issue

A couple of notes,

* Since the location info is now delegated to two child elements and the uri part has the css ellipsis, the ellipsis creates a gap (visually) when the underline is shown on :hover. I do not think it matters that much. Another solution would probably add complexity (eg. JS or having two a.location elements). Let me know if we want this solved in a different way.
* Do you think 'uriPath' is a good term here? 

Thomas
Attachment #823230 - Flags: review?(mihai.sucan)
Moving the ellipsis from the line number to the filename helps for some cases, but feels a bit like a workaround if there is no mechanism to retrieve the full file:line information. Would it be possible to make the column header resizable so that user can fetch the full information that way? And/or in the case that there is an ellipsis, serve the full information via mousehover in a tooltip?
> Would it be possible to make the column header resizable so that user can fetch the full information that way?

Today there are no header collumns. The console can display all kinds of information at once (net,css,js,security etc.) I guess it would be a bit convoluted to have generic header columns that would cover them all

> And/or in the case that there is an ellipsis, serve the full information via mousehover in a tooltip?

The full url is revealed in a tooltip when hovering the link today
Hello,
I would like to work on this bug. Can you please assign it to me?
Assignee: nobody → bhagya.ravikumar
Assignee: bhagya.ravikumar → thomas
Bhagya, this bug is already being worked on, so it's best to find another one to work on from the good first bug list[1].

[1]: https://wiki.mozilla.org/DevTools/GetInvolved#Mentored_and_Good_First_Bugs
Comment on attachment 823230 [details] [diff] [review]
920321-initial.patch

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

Patch looks good, Thomas. Thanks for your work!

I do not mind the rendering gap with the ellipsis and the underline when hovering. That seems to be a platform render bug - it should underline the ellipsis as well. Can you make a small web page test case and file a bug?

r+ with the comments below addressed.

::: browser/devtools/webconsole/webconsole.js
@@ +2605,5 @@
>     */
>    createLocationNode: function WCF_createLocationNode(aSourceURL, aSourceLine)
>    {
>      let locationNode = this.document.createElementNS(XHTML_NS, "a");
> +    let uriPathNode = this.document.createElementNS(XHTML_NS, "span");

nit: s/uriPath/filename/

@@ +2610,4 @@
>  
>      // Create the text, which consists of an abbreviated version of the URL
> +    // Scratchpad URLs should not be abbreviated.
> +    let uriPath;

filename?

@@ +2623,5 @@
>        fullURL = aSourceURL.split(" -> ").pop();
> +      uriPath = WebConsoleUtils.abbreviateSourceURL(fullURL);
> +    }
> +
> +    uriPathNode.className = "uri-path";

Maybe .filename? We only show the filename - or that is what we intend to do.

::: browser/themes/shared/devtools/webconsole.inc.css
@@ -69,2 @@
>    flex: 0 0 auto;
> -  align-self: flex-start;

Why not keep align-self?

@@ +81,5 @@
>  }
>  
> +.message > .location > .uri-path {
> +  text-overflow: ellipsis;
> +  text-align: right;

s/right/end/

end is preferred, it respects UI language direction.

@@ +87,5 @@
> +  white-space: nowrap;
> +}
> +
> +.message > .location > .line-number {
> +  flex: none;

s/none/0/
Attachment #823230 - Flags: review?(mihai.sucan) → review+
Attached patch 920321-r1.patch (obsolete) — Splinter Review
Addressing issues mentioned in comment #12
Attachment #823230 - Attachment is obsolete: true
Attachment #824890 - Flags: review?(mihai.sucan)
Thanks for the review Mihai

> That seems to be a platform render bug
it is intentional according to a comment in another thread (see below). As I understand it the spec is not clear about this. WebKit has the same behavior.

> Can you make a small web page test case and file a bug?
Created.. dupe: https://bugzilla.mozilla.org/show_bug.cgi?id=932926

Thomas
Comment on attachment 824890 [details] [diff] [review]
920321-r1.patch

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

Thanks for the quick update. Sorry, but some points from comment 12 still need to be addressed.

::: browser/devtools/webconsole/webconsole.js
@@ +2614,5 @@
>      let fullURL;
>      let isScratchpad = false;
>  
>      if (/^Scratchpad\/\d+$/.test(aSourceURL)) {
> +      filename = aSourceURL;

|filename| is undefined

(you forgot to change |let uriPath|)

::: browser/themes/shared/devtools/webconsole.inc.css
@@ -69,2 @@
>    flex: 0 0 auto;
> -  align-self: flex-start;

You did not answer my question about why this is removed?

@@ +81,5 @@
>  }
>  
> +.message > .location > .filename {
> +  text-overflow: ellipsis;
> +  text-align: right;

I suggested s/right/end/

@@ +87,5 @@
> +  white-space: nowrap;
> +}
> +
> +.message > .location > .line-number {
> +  flex: none;

I also suggested s/none/0/
Attachment #824890 - Flags: review?(mihai.sucan)
(In reply to Thomas Andersen from comment #14)
> Thanks for the review Mihai
> 
> > That seems to be a platform render bug
> it is intentional according to a comment in another thread (see below). As I
> understand it the spec is not clear about this. WebKit has the same behavior.
> 
> > Can you make a small web page test case and file a bug?
> Created.. dupe: https://bugzilla.mozilla.org/show_bug.cgi?id=932926

Thanks for the bug report!
Attached patch 920321-r1.patchSplinter Review
New version of r1
Attachment #824890 - Attachment is obsolete: true
Attachment #825500 - Flags: review?(mihai.sucan)
Sorry about that. 

The new patch should be up to date.
Not sure what happened here. Maybe I did not refresh (qrefresh) the patch before I submitted it?
Anyway, I will try to double check my patch before posting next time.

> You did not answer my question about why this is removed?
First time introduced to flex-boxes and I did not grasp the whole picture so I removed it as I saw it did nothing. Should have studied it better.
Comment on attachment 825500 [details] [diff] [review]
920321-r1.patch

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

Thanks for the updated patch! Good work. r+

I will land the patch later today.
Attachment #825500 - Flags: review?(mihai.sucan) → review+
(The trees are now open!)

Landed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/253d67e88e68
Whiteboard: [good first bug][lang=js,html,css][mentor=msucan] → [good first bug][lang=js,html,css][mentor=msucan][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/253d67e88e68
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js,html,css][mentor=msucan][fixed-in-fx-team] → [good first bug][lang=js,html,css][mentor=msucan]
Target Milestone: --- → Firefox 28
Whiteboard: [good first bug][lang=js,html,css][mentor=msucan] → [good first bug][lang=js,html,css][mentor=msucan][good first verify]
Duplicate of this bug: 968953
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.