Opened 12 years ago
Closed 12 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)
Change History (14)
comment:1 Changed 12 years ago by
| Milestone: | tbd → 1.4 |
|---|---|
| Version: | 1.3.0 → 1.3.1 |
Changed 12 years ago by
| Attachment: | RichText_selection_cleanup.patch added |
|---|
comment:2 Changed 12 years ago by
| Summary: | Editor: Cleanup and bugfixes → [PATCH][CCLA] Editor: Cleanup and bugfixes |
|---|
comment:3 Changed 12 years ago by
| Keywords: | bill liucougar added |
|---|
comment:4 Changed 12 years ago by
looks pretty good, there are some minor things:
- 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
- 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 12 years ago by
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 12 years ago by
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
comment:7 Changed 12 years ago by
I think the avoidance of dojo.IE was deliberate (feature detection over browser detection)
comment:8 Changed 12 years ago by
Except the detection fails for Opera, as it has attributes that *look* like IE but do not act like it at all.
Changed 12 years ago by
| Attachment: | RichText_latest.patch added |
|---|
Updated RichText? patch with some other JSLint cleanups (unused vars, whitespace stuff)
comment:10 Changed 12 years ago by
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
(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
Patches for RichText? and Selection code.