Closed Bug 56541 Opened 24 years ago Closed 24 years ago

Can't OK the Advanced edit dialog

Categories

(Core :: DOM: Editor, defect, P2)

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9

People

(Reporter: sfraser_bugs, Assigned: cmanske)

References

Details

(Keywords: regression)

Attachments

(2 files)

Fix for bug 56541
1.46 KB, patch
Details | Diff | Splinter Review
The correct bug fix
889 bytes, patch
Details | Diff | Splinter Review 
Edit a page with a link. double-click on the link to bring up the link properties 
dialog. In that dialog, hit the 'Advanced edit' button. Without making any 
changes, click OK in the Advanced edit button. Nothing happens. Clicking cancel 
dismisses the dialog.

I see this JS errors on clicking ok:

===============[ Setting and Updating HTML ]===============
JavaScript error: 
chrome://editor/content/EdAEHTMLAttributes.js line 82: HTMLRAttrs is not defined
Same happens when editing image properties. This basically makes the dialog 
useless.
Keywords: rtm
Priority: P3 → P2
Summary: Can't OK the Advanced edit dialog when editing a link → Can't OK the Advanced edit dialog
I'll take a look at this over the weekend. Changing assignment.
Assignee: brade → rcassin
Fix in hand.
Status: NEW → ASSIGNED
Whiteboard: [FIX IN HAND]

    
    

    
  
rtm+ this. ryan, please solicit reviews from ben and brade.

Whiteboard: [FIX IN HAND] → [rtm+][FIX IN HAND]

    
    

    
  
bugs can't be +'d until they have r=/sr=
Whiteboard: [rtm+][FIX IN HAND] → [FIX IN HAND]

    
  
Keywords: patch
OS: Mac System 8.5 → All
Whiteboard: [FIX IN HAND] → [rtm+ NEED INFO]patch attached

    
    

    
  
r=brade
Ben--can you review this too?


    
    

    
  
It has now been a week. I assume you wanted Ben Goodger and not Ben B. to do a
review?


    
    

    
  
hello??

Adding brendan@netscape.com to cc to get some kind of resolution here.

I am also changing the marking from [rtm+ need info] to [rtm need info], since
the former state doesn't really exist.

Whiteboard: [rtm+ NEED INFO]patch attached → [rtm NEED INFO]patch attached r=brade, NEED SR=

    
    

    
  
Ben Goodger has looked at the fix; I *think* he was ok with the fixes proposed 
but there are other serious bugs that need to be addressed in this bug (if I 
remember correctly).

Reassign bug to Ben so he'll maybe respond here sooner.  :-)

Assignee: rcassin → ben
Status: ASSIGNED → NEW

    
    

    
  
You need to send Ben email directly not re-assign to him.  I'll do that for you.


    
    

    
  
My question wrt this patch was 'is attribute removal broken by this patch?', 
because it is removing references to the arrays that held attributes which were 
to be removed. 

The answer I received was 'yes, attribute removal is broken, but the dialog now 
closes.' 

This answer may be sufficient for the Netscape branch. If so, please find 
someone else to give it a=. For the trunk, I this patch cannot pass review as it 
stands, as it does not really do anything to fix the dialog (which is completely 
broken). (the arrays that the code that this patch modifies were removed between 
rev 1.13 and 1.14, I don't see how the dialog could have even worked after that 
mod). 



    
  
Keywords: relnoteRTM

    
    

    
  
OK, re-assigning to former owner.  Ben doesn't approve of the patch.

Assignee: ben → rcassin

    
    

    
  
This isn't going to pass PDT muster for RTM. Moving to mozilla.

Keywords: rtm → regression
Whiteboard: [rtm NEED INFO]patch attached r=brade, NEED SR=
Target Milestone: --- → mozilla0.9

    
    

    
  
*** Bug 59061 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 58934 has been marked as a duplicate of this bug. ***

    
    

    
  
Adding to Composer sectoin of Release Notes. 

Is the workaround to edit source rather than use Advanced Edit in the Link 
Properties dialog?


    
    

    
  
*** Bug 60148 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 60322 has been marked as a duplicate of this bug. ***

    
    

    
  
I already have a bug and fix for this issue and will be checking in with other
Advanced Edit dialog fixes.
Assignee: rcassin → cmanske

    
    

    
  
*** Bug 60796 has been marked as a duplicate of this bug. ***

    
    

    
  


  

    
    

    
  
New fix was attached.
We do want to accumulate a list of attributes to remove, so the fix is to restore
the declaration of the global arrays that were previously removed.

Status: NEW → ASSIGNED

    
    

    
  
I presume this patch is just to fix the bug(being able to close the Advanced
Property Editor dialog), and doesnt actually fix the problem of not removing
attributes when it should, which got broken in v1.14 of EdAvancedEdit.js ...and
that the other Advanced Edit Dialog fixes referred to by Charles Manske will
correct this ?

    
    

    
  
No, the code to remove an attribute already exists, but couldn't work because the
array of attributes to remove didn't exist. This fix makes removing attributes
work correctly. (I just did a quick test and the code in my tree it sets and
removes attributes successfully.)


    
    

    
  
I retract my previous comment -- There are other changes to EdAEHTMLAttributes.js
that are needed to make remove attribute work correctly.
Thanks, Gary.


    
    

    
  
*** Bug 63814 has been marked as a duplicate of this bug. ***

    
    

    
  
Finally checked in fixes. Using the CSS editing should be more thoroughly tested.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED

    
  
Status: RESOLVED → VERIFIED

    
    

    
  
verified in 1/17 build.

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: