Opened 10 years ago

Closed 10 years ago

#9520 closed defect (fixed)

[PATCH][CCLA] Editor: Cleanup and bugfixes

Reported by: Jared Jurkiewicz Owned by: Jared Jurkiewicz
Priority: high Milestone: 1.4
Component: Editor Version: 1.3.1
Keywords: bill, liucougar Cc:
Blocked By: Blocking:

Description

1.) The editor could use a bit of refactoring to make certain updates easier. Namely, the RichText?.execCommand could be split into a simpler function that calls over-rides in the case of functions we over-ride and a default in the case of those we do not. Cleaner separation and gets rid of an ugly if/else block.

2.) There are several bugs in the selection code for the Opera browser. Opera works differently from FF/Safari AND IE. The main problems I see there are:

a.) Opera has a document.selection attribute, but it does not work like IE. The selection code uses presence of this attribute to determine if the browser is in IE or not. This causes errors and needs to be updated.

b.) selectChildrenElements doesn't work right on Opera. Opera's selection API is odd and doesn't conform to Range exactly.

3.) The documentation of selection (function docs), and such do not conform to API guidelines.

I'm taking a stab at fixing all these, so assigning to myself.

Attachments (4)

RichText_selection_cleanup.patch (21.6 KB) - added by Jared Jurkiewicz 10 years ago.
Patches for RichText? and Selection code.
RichText.patch (12.3 KB) - added by Jared Jurkiewicz 10 years ago.
Updated patch isolated to RichText?
Selection.patch (10.0 KB) - added by Jared Jurkiewicz 10 years ago.
Updated selection patch.
RichText_latest.patch (12.9 KB) - added by Jared Jurkiewicz 10 years ago.
Updated RichText? patch with some other JSLint cleanups (unused vars, whitespace stuff)

Download all attachments as: .zip

Change History (14)

comment:1 Changed 10 years ago by Jared Jurkiewicz

Milestone: tbd1.4
Version: 1.3.01.3.1

Changed 10 years ago by Jared Jurkiewicz

Patches for RichText? and Selection code.

comment:2 Changed 10 years ago by Jared Jurkiewicz

Summary: Editor: Cleanup and bugfixes[PATCH][CCLA] Editor: Cleanup and bugfixes

comment:3 Changed 10 years ago by Jared Jurkiewicz

Keywords: bill liucougar added

comment:4 Changed 10 years ago by liucougar

looks pretty good, there are some minor things:

  1. you added dojo.isIE so that opera will take the same code path as non-IE browsers. If we have dojo.isIE there, then we don't need to list "dojo.doc.selection" (or similar) any more
  2. this comment: "Opera's selectAllChildren doesn't seem to work right against <body> nodes and possibly others ... so we have to do this differently, and not entirely to how W3C" is actually not accurate, what you have in that if block is valid W3C range API calls, which (I believe) works just fine in firefox.

comment:5 Changed 10 years ago by Jared Jurkiewicz

Verified that doc.selection is always present on IE, so removed it as part of the check, dojo.isIE is fine.

The second part on opera, well, selectAllChildren I think is also part of the range API, but doesn't work right on Opera. The actual calls I make work fine there. I haven't tried it in FF, though. Didn't want to change working code if I could avoid it (In case what I did for opera doesn't work, oy.)

comment:6 Changed 10 years ago by liucougar

I think just say "Opera's selectAllChildren doesn't seem to work right against <body> nodes and possibly others ... so we have to do this with W3C Range primitive API calls" in the comment should be enough

Changed 10 years ago by Jared Jurkiewicz

Attachment: RichText.patch added

Updated patch isolated to RichText?

Changed 10 years ago by Jared Jurkiewicz

Attachment: Selection.patch added

Updated selection patch.

comment:7 Changed 10 years ago by Adam Peller

I think the avoidance of dojo.IE was deliberate (feature detection over browser detection)

comment:8 Changed 10 years ago by Jared Jurkiewicz

Except the detection fails for Opera, as it has attributes that *look* like IE but do not act like it at all.

comment:9 Changed 10 years ago by Jared Jurkiewicz

Checked in changeset for selection under: [19368]

Changed 10 years ago by Jared Jurkiewicz

Attachment: RichText_latest.patch added

Updated RichText? patch with some other JSLint cleanups (unused vars, whitespace stuff)

comment:10 Changed 10 years ago by Jared Jurkiewicz

Resolution: fixed
Status: newclosed

(In [19503]) Checking in cleanup to RichText? code. Mainly JSLint fixes + a refactor of the execCommand function so that it's easier to add in custom behaviors for 'builtin' commands if needed. Tested using DOH Robot tests and manual tests on FF2, FF3.5, Safari 3.2.3, IE 7, IE 8. IE 6 testing underway. \!strict fixes #9520

Note: See TracTickets for help on using tickets.