Closed Bug 1527109 Opened 6 years ago Closed 3 years ago

Remove ellipseString function and use text-overflow:ellipsis instead

Categories

(Toolkit :: Printing, enhancement, P5)

Product:
Toolkit
Component:
Printing
Type:
enhancement

Tracking

()

Status:
RESOLVED INVALID
Tracking Flags:
Tracking Status
firefox67 --- affected

People

(Reporter: jaws, Unassigned)

Details

(Keywords: good-first-bug)

Attachments

(2 obsolete files)

Hi,

I'd like to start working on this bug (it would be my first bug). Could I get assigned?

(In reply to Adam Czyzewski from comment #1)

Hi,

I'd like to start working on this bug (it would be my first bug). Could I get assigned?

Hi Adam, it would be great for you to work on this. Please let me know if you have any questions. You can follow the steps here to get your local build set up: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

Since this bug doesn't involved changing any C++ you can use an artifact build (still follow the instructions above) to make your build time much shorter: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds

Assignee: nobody → adam.czyzewski
Status: NEW → ASSIGNED

Hi Adam, have you been able to make progress on this bug? Do you have any questions for me?

Flags: needinfo?(adam.czyzewski)

Hi, thanks for checking up with me.
So far I set up my build and plan to solve the bug on the weekend. Everything seems straightforward for now :)

Flags: needinfo?(adam.czyzewski)

I've started working on the bug, but I'm not sure what the desired behaviour should be.

Right now the ellipseString method is used to modify docTitle and docURLStr (https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/toolkit/components/printing/content/printProgress.js#100). Before any modification thought, it performs a few checks (for empty string, for length of the string).

Also, we can add "..." before the docURL string, by setting "doFront" parameter to true. Do we want to preserve that?

Other than that, is using text-overflow:ellipsis in the css file what you have in mind?

Flags: needinfo?(jaws)
Assignee: adam.czyzewski → nobody
Status: ASSIGNED → NEW

(In reply to Adam Czyzewski from comment #5)

I've started working on the bug, but I'm not sure what the desired behaviour should be.

Right now the ellipseString method is used to modify docTitle and docURLStr (https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/toolkit/components/printing/content/printProgress.js#100). Before any modification thought, it performs a few checks (for empty string, for length of the string).

Also, we can add "..." before the docURL string, by setting "doFront" parameter to true. Do we want to preserve that?

Other than that, is using text-overflow:ellipsis in the css file what you have in mind?

Yeah, text-overflow:ellipsis in the CSS file is what I had in mind. We should try to preserve the doFront behavior for now. This article shows a way that we can use text-overflow:ellipsis with direction to place the ellipsis at the front of a string, https://davidwalsh.name/css-ellipsis-left

When changing the direction as the above hack describes, we'll also need to take care to apply it in the reverse for RTL strings.

You can use the https://developer.mozilla.org/en-US/docs/Web/CSS/:-moz-locale-dir(rtl) pseudo-class to check if the locale is in RTL. For example,

#docTitle,
#docURLStr {
    /* Standard CSS ellipsis */
    white-space: nowrap;
    overflow: hidden;
    text-overflow: ellipsis;
    width: 200px;    

    /* Beginning of string */
    direction: rtl;
    text-align: left;
}

#docTitle:-moz-locale-dir(rtl),
#docURLStr:-moz-locale-dir(rtl) {
    direction: ltr;
    text-align: right;
}

I see you have unassigned the bug. Sorry for the delayed response, yesterday was a holiday for us in the US. Does this help you enough to pick the bug back up?

Flags: needinfo?(jaws) → needinfo?(adam.czyzewski)

No worries! I've sent a bug patch for another bug, so for now I'll wait for the feedback. I unassigned the bug, so that in the meantime the bug can be picked by somebody if they want to fix it!

Flags: needinfo?(adam.czyzewski)

Hi, I'd would like to take this bug and try to fix it

(In reply to young.oh from comment #8)

Hi, I'd would like to take this bug and try to fix it

Great, please let me know of any questions you may have. I'll follow up with you in a couple days to see where you have gotten.

Assignee: nobody → yg.oh
Status: NEW → ASSIGNED

(In reply to [away 2/21-2/26] Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me) from comment #6)

(In reply to Adam Czyzewski from comment #5)

I've started working on the bug, but I'm not sure what the desired behaviour should be.

Right now the ellipseString method is used to modify docTitle and docURLStr (https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/toolkit/components/printing/content/printProgress.js#100). Before any modification thought, it performs a few checks (for empty string, for length of the string).

Also, we can add "..." before the docURL string, by setting "doFront" parameter to true. Do we want to preserve that?

Other than that, is using text-overflow:ellipsis in the css file what you have in mind?

Yeah, text-overflow:ellipsis in the CSS file is what I had in mind. We should try to preserve the doFront behavior for now. This article shows a way that we can use text-overflow:ellipsis with direction to place the ellipsis at the front of a string, https://davidwalsh.name/css-ellipsis-left

When changing the direction as the above hack describes, we'll also need to take care to apply it in the reverse for RTL strings.

You can use the https://developer.mozilla.org/en-US/docs/Web/CSS/:-moz-locale-dir(rtl) pseudo-class to check if the locale is in RTL. For example,

#docTitle,
#docURLStr {
    /* Standard CSS ellipsis */
    white-space: nowrap;
    overflow: hidden;
    text-overflow: ellipsis;
    width: 200px;    

    /* Beginning of string */
    direction: rtl;
    text-align: left;
}

#docTitle:-moz-locale-dir(rtl),
#docURLStr:-moz-locale-dir(rtl) {
    direction: ltr;
    text-align: right;
}

I see you have unassigned the bug. Sorry for the delayed response, yesterday was a holiday for us in the US. Does this help you enough to pick the bug back up?

Hi, I finished setting up firefox build and briefly went through the code. I found https://searchfox.org/mozilla-central/source/toolkit/components/printing/content/simplifyMode.css in the same directory, Can you confirm if this is where the above CSS styles should go?

Flags: needinfo?(jaws)

That file is only used to simplify the page before printing, and that stylesheet is applied to the page directly, so it wouldn't work here to apply it to the dialogs. You should create a new stylesheet and add references to it from printPreviewProgress.xul and printProgress.xul.

Flags: needinfo?(jaws)

Ok.. now I'm having a little trouble preserving doFront behaviour... There is an element dialog.title which is assigned either docTitle or docURL as its value. And doFront occurs only when docURL has length greater than 64. I'm thinking of styling dialog.title dynamically in js only in that specific case.

Flags: needinfo?(jaws)

I think I somehow figured it out. Please review the patch!

Flags: needinfo?(jaws)
Attached patch 1527109.patch (obsolete) — — Splinter Review

Hi,here is the revised patch.

Attachment #9047999 - Attachment is obsolete: true

(In reply to young.oh from comment #15)

Created attachment 9048066 [details] [diff] [review]
1527109.patch

Hi,here is the revised patch.

Please resubmit this through phabricator as you did the first version of this patch.

Flags: needinfo?(yg.oh)
Attachment #9047999 - Attachment is obsolete: false
Flags: needinfo?(yg.oh)

Resubmitted it again with phabricator!

Attachment #9048066 - Attachment is obsolete: true

I made the change to both jar.mn and xul files, however I still could observe the long string length > 64 is not getting ellipsed. I expected to see dots at the end of the string but the whole string appears in the title label. I checked that CSS on #dialog.title does work by applying some random text color. I attempted the solution (CSS display property) on StackOverflow, direct styling in xul, and none of them worked. I'm suspecting <label /> element does not work with text-overflow and other props. Can you provide further guidance on this? Thanks

Flags: needinfo?(jaws)
Attachment #9047999 - Attachment is obsolete: true
Assignee: yg.oh → nobody
Status: ASSIGNED → NEW

Hi! I would like to work on this.

I would like to work on this if it's available, thanks.

(In reply to Obayagbona Uwagbae Alexander from comment #20)

I would like to work on this if it's available, thanks.

Hi, you may work on this but I will wait to assign it until a patch has been attached.

Flags: needinfo?(jaws)

Okay, no problem.
Do I have to do this using CSS? If I find a way to implement it using JS, can I do that?

Flags: needinfo?(jaws)

Can I work on this bug?

Thanks
Sonia

Flags: needinfo?(mconley)

Hi Sonia,

Our printing UI is going through some changes right now, so I'm going to redirect this question to someone on the team working on that to see if this bug still makes sense to work on. If the answer is "yes", then I can mentor you on this.

Flags: needinfo?(mconley) → needinfo?(mstriemer)
Mentor: jaws
Flags: needinfo?(jaws)

Is this bug fixed, it looks so...
If not, may I work on it?

thanks,
Kian

Flags: needinfo?(jaws)

Hi, I am an outreachy applicant. Is this project open?
Can I work on this?

This code was removed in bug 1745452 so no need to update it

Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(mstriemer)
Flags: needinfo?(jaws)
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: