Status

()

Firefox
Menus
5 years ago
4 years ago

People

(Reporter: André Reinald, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Text is appended to a temporary variable before being returned.
This variable is initialized on every occurrence of an image which has its alt field filled.
How exactly are you doing this conversion?
Flags: needinfo?(areinald)
(Reporter)

Comment 2

5 years ago
Created attachment 718463 [details] [diff] [review]
Patch to fix the issue, the alt field text is appended to the "text" variable.
Attachment #718463 - Flags: review?(paul)
Flags: needinfo?(areinald)
Component: DOM → General
Product: Core → Firefox
(Reporter)

Comment 3

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #1)
> How exactly are you doing this conversion?

I don't know when this code is called, I just noticed this obvious bug while reading the code. It may be dead code, and never be called though.
(Reporter)

Comment 4

5 years ago
The code seems to simply walk the sub-tree starting at a specified node. No conversion is performed on the way. Only white space is removed before returning the text buffer.
Looks like this is called from some context menu methods.  But what I was really after was which code you were talking about; I didn't realize you already had a patch for it.  ;)
(Reporter)

Updated

5 years ago
Assignee: nobody → areinald

Comment 6

5 years ago
Comment on attachment 718463 [details] [diff] [review]
Patch to fix the issue, the alt field text is appended to the "text" variable.

I can't review this (wrong :paul I guess).
Attachment #718463 - Flags: review?(paul)
Comment on attachment 718463 [details] [diff] [review]
Patch to fix the issue, the alt field text is appended to the "text" variable.

Paul, you actually have the only blame in this function after the migration to Mercurial, which is why I assume you were fingered.

The blame on the original code goes back to CVS rev 1.1, which was blakeross doing initial Firefox UI development back in 2002.

I'm going to claim that I'm qualified to review this code.  ;)

André, do you have push access, or do you need this pushed to inbound?  In the latter case, can you please include the bug number in your commit message?
Attachment #718463 - Flags: review+
This code is pretty broken in other ways, too:
- "altText && altText != """ is redundant
- bug 539193
- should use .trim() rather than those regexes to strip whitespace
- seems to only be used to gather the text for a link element, so it could probably be simplified
Gavin, you want to take over the reviews here?  You're probably a better fit anyway.  ;)

Comment 10

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #7)
> Paul, you actually have the only blame in this function after the migration
> to Mercurial, which is why I assume you were fingered.
> 
> The blame on the original code goes back to CVS rev 1.1, which was blakeross
> doing initial Firefox UI development back in 2002.

Another Paul. Back then, I didn't even know what internet was :)
No, definitely you.  The change after Blake's stuff (which was Blake, not you) was bug 710878.

Comment 12

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #11)
> No, definitely you.  The change after Blake's stuff (which was Blake, not
> you) was bug 710878.

Damn, you're right. Forgot about this one.
(Reporter)

Comment 13

5 years ago
Created attachment 719040 [details] [diff] [review]
Replace 1st patch which was flawed and takes into account gavin's comment.

At second thought, the "text = altText" was there on purpose: it meant to use the alt text of an image node, and only use that. But that was probably meant to work only when (depth == 1), not for an image further down the tree.

So either I add (depth == 1) as a condition and keep the break instruction, or I remove the break too. My previous patch was halfway and would result in "word1 word2 alt1" instead of "word1 word2 alt1 word3 alt2 word4"

I picked the 2nd option, and removed the break. Plus I took into account Gavin's comment and used trim instead of 2 regex replaces.
Attachment #718463 - Attachment is obsolete: true
Attachment #719040 - Flags: review?(paul)
Attachment #719040 - Attachment is patch: true
I think we should probably step back here and figure out what this function should be doing, given that it's extremely old and crufty.

It seems to only be used for:
- obtaining a default file name when you alt+click a link to save it (and have browser.altClickSave set to true)
- obtaining a default file name when using "save link as"
- pre-filling the "Name" field when using "bookmark this link"

Wouldn't just using node.textContent be sufficient for most of these cases? We could also optionally support checking for whether "node" is an image and using .alt in that case. Since this is just used as a default file/bookmark name, it really doesn't have to be perfect.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #14)
> It seems to only be used for:
> - obtaining a default file name when you alt+click a link to save it (and
> have browser.altClickSave set to true)
> - obtaining a default file name when using "save link as"

And even in these cases it's very rarely actually used, since the code in contentAreaUtils.js getDefaultFilename() uses it only as a fallback in case the URI being saved is not an nsIURL/doesn't have a .fileName. And in the "save link as", we only even fall back to that code if we hit NS_ERROR_SAVE_LINK_AS_TIMEOUT.

Comment 16

5 years ago
Comment on attachment 719040 [details] [diff] [review]
Replace 1st patch which was flawed and takes into account gavin's comment.

(cancelling review until Gavin's comment is addressed)
Attachment #719040 - Flags: review?(paul)
Duplicate of this bug: 539193
The core issue that led to this bug being filed was fixed in bug 906465, but there is still some cleanup needed here (comment 8/comment 14)
Assignee: areinald → nobody
Summary: While converting HTML to text, if an "alt" field is filled for an image element, the buffer is emptied → clean up gatherTextUnder

Updated

4 years ago
Component: General → Menus

Updated

4 years ago
Blocks: 306937
You need to log in before you can comment on or make changes to this bug.