Closed
Bug 215406
Opened 21 years ago
Closed 21 years ago
Remove all text styles menu item and key binding both no longer working
Categories
(Core :: DOM: Editor, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla1.7alpha
People
(Reporter: raw, Assigned: glazou)
Details
Attachments
(1 file)
1.75 KB,
patch
|
mozeditor
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030718 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030718 The re-implementation of style support in 1.5a seems to have broken the Format->Remove All Text Styles menu item, at least on Windows. Neither the menu item nor the key binding seemt to do anything. Toggling text styles using other formatting commands (bold, italic, fixed width) work correctly, but clearing them all in one swell foop no longer works. Reproducible: Always Steps to Reproduce: 1. Create new composer page 2. Type a line of text, applying text styles to some of the text 3. Select the line 4. Format->Remove All Text Styles Actual Results: Nothing changes in the selected text. Expected Results: Remove all text styles from the selected text.
Comment 1•21 years ago
|
||
It'd be nice to get a timeframe for when this broke.
Assignee: composer → mozeditor
Component: Editor: Composer → Editor: Core
QA Contact: petersen → sairuh
Comment 3•21 years ago
|
||
Teh culprit is: nsHTMLEditorStyle.cpp 1.53 glazman%netscape.com Jun 17 01:45 Removing text styles in CSS mode was potentially removing too much if the styles were added in HTML mode The fix creates a span element that will carry the inline styles and class of the HTML element to be removed, if any. b=202037, r=kaie, sr=dmose Code was added to preserve additional inline style attributes on html style nodes that were going away. However, there is no check to see if this additional style is in fact the style we are trying to remove. Short fixes: backout change, or remove the style attribute copying but keep the class attribute checking. More correct fix: check to see if style is one we are trying to remove, and if so don't preserve it. Note that (aProperty == nsnull) implies that *all* styles are being removed. Daniel, do you want to fix this, or should I?
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•21 years ago
|
||
Taking
Assignee: mozeditor → daniel
Status: ASSIGNED → NEW
Priority: -- → P1
Assignee | ||
Comment 5•21 years ago
|
||
excellent diagnosis from Joe, as usual... Easy fix pending.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.7alpha
Assignee | ||
Comment 6•21 years ago
|
||
Assignee | ||
Comment 7•21 years ago
|
||
Comment on attachment 139556 [details] [diff] [review] fix #1 Joe, can you review this simple fix, please ? David, can you sr please ? Thanks!
Attachment #139556 -
Flags: superreview?(dbaron)
Attachment #139556 -
Flags: review?(mozeditor)
Comment 8•21 years ago
|
||
Comment on attachment 139556 [details] [diff] [review] fix #1 I'm confused about this part: + if (aProperty && + (hasStyleAttr || hasClassAtrr)) { Does this mean we will remove spans that have class attribute when we are removing all styles? Is that ok?
Assignee | ||
Comment 9•21 years ago
|
||
Right. I think it is fair to remove class on inline-level nodes when we remove all styles. It _may_ remove styles that are not textual (a border for instance) but we have no way to remove the "textual" styles attached through a class while keeping the others...
Comment 10•21 years ago
|
||
Comment on attachment 139556 [details] [diff] [review] fix #1 ok, sounds good. r=mozeditor
Attachment #139556 -
Flags: review?(mozeditor) → review+
Attachment #139556 -
Flags: superreview?(dbaron) → superreview+
Assignee | ||
Comment 11•21 years ago
|
||
checked in (trunk)
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•